Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix win32 breakage 18 dec #9399

Merged
merged 7 commits into from
Dec 20, 2019
Merged

Fix win32 breakage 18 dec #9399

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3d8ef7a
ifdef within a macro results in undefined behavior
wrowe Dec 18, 2019
2fba5da
Permit FooFactor to be instantiated
sunjayBhatia Dec 18, 2019
5e313e8
Add spelling words, correct non-words
wrowe Dec 18, 2019
1a46407
Remove reduncies as suggested by jmarantz
sunjayBhatia Dec 19, 2019
0b4daa3
Merge branch 'master' into fix-win32-breakage-18-dec
wrowe Dec 20, 2019
90f7117
Drop misspellings as suggested by zuercher
wrowe Dec 20, 2019
5baa115
Simplify as suggested by zuercher
wrowe Dec 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions test/common/common/mem_block_builder_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,28 +68,24 @@ TEST(MemBlockBuilderTest, AppendUint32) {
EXPECT_EQ(0, mem_block.capacity());
}

TEST(MemBlockBuilderTest, AppendOneTooMuch) {
MemBlockBuilder<uint8_t> mem_block(1);
mem_block.appendOne(1);
EXPECT_DEATH({ mem_block.appendOne(2); },
#ifdef ENVOY_CONFIG_COVERAGE
"" // For some reason, this test under coverage generates a list of testdata/*.
// For some reason, this test under coverage generates a list of testdata/*.
static const char expected_death_regex[] = "";
#else
".*insufficient capacity.*"
static const char expected_death_regex[] = ".*insufficient capacity.*";
#endif
);

TEST(MemBlockBuilderTest, AppendOneTooMuch) {
MemBlockBuilder<uint8_t> mem_block(1);
mem_block.appendOne(1);
EXPECT_DEATH({ mem_block.appendOne(2); }, expected_death_regex);
}

TEST(MemBlockBuilderTest, AppendDataTooMuch) {
MemBlockBuilder<uint8_t> mem_block(1);
const uint8_t foo[] = {1, 2};
EXPECT_DEATH({ mem_block.appendData(absl::MakeConstSpan(foo, ABSL_ARRAYSIZE(foo))); },
#ifdef ENVOY_CONFIG_COVERAGE
"" // For some reason, this test under coverage generates a list of testdata/*.
#else
".*insufficient capacity.*"
#endif
);
expected_death_regex);
}

} // namespace Envoy
2 changes: 1 addition & 1 deletion test/common/config/metadata_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class TypedMetadataTest : public testing::Test {

struct Bar : public TypedMetadata::Object {};

class FooFactory : public TypedMetadataFactory::TypedMetadataFactory {
class FooFactory : public Envoy::Config::TypedMetadataFactory {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? It seems lexically obvious we are in a namespace where TypedMetadataFactory should be usable without qualification. If this cannot work I wonder what other things will fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this commit log;

TypedMetadataFactory::TypedMetadataFactory remains too abstract, and didn't have base class methods name or parse.

We are quite confused as well, this is the error output of the original source;

ERROR: C:/workspace/envoy/test/common/config/BUILD:261:1: C++ compilation of rule '//test/common/config:metadata_test_lib_internal_only' failed (Exit 2)
test/common/config/metadata_test.cc(63): error C2516: 'Envoy::Config::TypedMetadataFactory::{ctor}': is not a legal base class
test/common/config/metadata_test.cc(65): error C3668: 'Envoy::Config::`anonymous-namespace'::TypedMetadataTest::FooFactory::name': method with override specifier 'override' did not override any base class methods
test/common/config/metadata_test.cc(68): error C3668: 'Envoy::Config::`anonymous-namespace'::TypedMetadataTest::FooFactory::parse': method with override specifier 'override' did not override any base class methods
test/common/config/metadata_test.cc(54): error C2664: 'Envoy::Registry::InjectFactory<Envoy::Config::TypedMetadataFactory>::InjectFactory(const Envoy::Registry::InjectFactory<Envoy::Config::TypedMetadataFactory> &)': cannot convert argument 1 from 'Envoy::Config::`anonymous-namespace'::TypedMetadataTest::FooFactory' to 'Base &'
        with
        [
            Base=Envoy::Config::TypedMetadataFactory
        ]
.\test/test_common/registry.h(17): note: see declaration of 'Envoy::Registry::InjectFactory<Envoy::Config::TypedMetadataFactory>::InjectFactory'

Things that are confusing, why promote TypedMetadataFactory:: to Envoy::Config:: in these contexts? Why the anon namespace? Suggestions and observations welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work if you just extend TypedMetadataFactory instead of TypedMetadataFactory::TypedMetadataFactory? The latter seems to be nonsense, since there's no TypedMetadataFactory namespace in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zuercher agreed, that was the issue. Will push the fix first thing Friday.

public:
const std::string name() const override { return "foo"; }
// Throws EnvoyException (conversion failure) if d is empty.
Expand Down
6 changes: 3 additions & 3 deletions tools/clang_tools/api_booster/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Upgrade a single Envoy C++ file to the latest API version.
//
// Currently this tool is a WiP and only does inference of .pb[.validate].h
// Currently this tool is a WIP and only does inference of .pb[.validate].h
// #include locations. This already exercises some of the muscles we need, such
// as AST matching, rudimentary type inference and API type database lookup.
//
Expand Down Expand Up @@ -112,7 +112,7 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback,
return true;
}

// Visitor callback for end of a compiation unit.
// Visitor callback for end of a compilation unit.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove "compiation" from the dictionary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on our fork. Will have to merge to master first.

void handleEndSource() override {
// Dump known API header paths to stdout for api_boost.py to rewrite with
// (no rewriting support in this tool yet).
Expand Down Expand Up @@ -161,7 +161,7 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback,
// Die hard if we don't have a useful proto type for something that looks
// like an API type(modulo a short whitelist).
std::cerr << "Unknown API type: " << proto_type_name << std::endl;
// TODO(htuch): mabye there is a nicer way to terminate AST traversal?
// TODO(htuch): maybe there is a nicer way to terminate AST traversal?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest you might be better off tracking the commit that added these, and work backwards to correct them (these are simply files that trip up verify on our roll-up of work, we didn't look for every misspelling.) These are only the few that we found, this was obviously pushed no-verify, and the spelling words added after the fact.

(see: [https://github.com//pull/9241] )

::exit(1);
}

Expand Down
10 changes: 10 additions & 0 deletions tools/spelling_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ API
ASAN
ASCII
ASSERTs
AST
AWS
BACKTRACE
BSON
Expand Down Expand Up @@ -47,6 +48,7 @@ CTXs
CVC
CVE
CX
CYGWIN
DER
DESC
DFATAL
Expand All @@ -56,7 +58,9 @@ DNS
DRYs
DS
DST
DW
EADDRINUSE
EADDRNOTAVAIL
EAGAIN
ECDH
ECDHE
Expand Down Expand Up @@ -90,6 +94,7 @@ FCDS
FFFF
FIN
FIPS
FIRSTHDR
FQDN
FREEBIND
FUZZER
Expand Down Expand Up @@ -149,6 +154,7 @@ LDS
LEV
LF
LHS
LLVM
LRS
MB
MD
Expand All @@ -164,6 +170,7 @@ NACKs
NBF
NBSP
NDEBUG
NEXTHDR
NGHTTP
NOAUTH
NODELAY
Expand Down Expand Up @@ -298,6 +305,7 @@ VLOG
VM
WASM
WAVM
WIP
WKT
WRR
WS
Expand Down Expand Up @@ -525,6 +533,7 @@ evwatch
exe
execlp
expectable
extrahelp
faceplant
facto
failover
Expand Down Expand Up @@ -644,6 +653,7 @@ lenenc
lexically
libc
libevent
libtool
libstdc
lifecycle
lightstep
Expand Down
2 changes: 1 addition & 1 deletion tools/type_whisperer/api_type_db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace TypeWhisperer {

// We don't expose the raw API type database to consumers, as this requires RTTI
// and this may be linked in environments where this is not available (e.g.
// libtooling binaries).
// libtool binaries).
class ApiTypeDb {
public:
static absl::optional<std::string> getProtoPathForType(const std::string& type_name);
Expand Down