Fix win32 breakage 18 dec by wrowe · Pull Request #9399 · envoyproxy/envoy

wrowe · 2019-12-18T20:05:37Z

Description:
This corrects the newest breakage on the Windows platform introduced in December.

Risk Level: Low
Testing: Local on Windows msvc and Linux gcc
Docs Changes: n/a
Release Notes: n/a

- gcc and clang may handle this but msvc does not - see section 16.3/11 of the standard Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

On MSVC we cannot derive from TypedMetadataFactory::TypedMetadataFactory as it remains too abstract, and didn't have base class methods name or parse. Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

This restores master to passing the validation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

jmarantz

lgtm modulo minor comments, which can be addressed in your senior approver round.

jmarantz · 2019-12-19T01:36:41Z

test/common/config/metadata_test.cc

  struct Bar : public TypedMetadata::Object {};

-  class FooFactory : public TypedMetadataFactory::TypedMetadataFactory {
+  class FooFactory : public Envoy::Config::TypedMetadataFactory {


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?

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.

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.

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

test/common/common/mem_block_builder_test.cc

jmarantz · 2019-12-19T01:40:43Z

@zuercher for senior maintainer approval.

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

zuercher · 2019-12-19T23:29:06Z

tools/clang_tools/api_booster/main.cc

  }

-  // Visitor callback for end of a compiation unit.
+  // Visitor callback for end of a compilation unit.


Can you remove "compiation" from the dictionary.

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

zuercher · 2019-12-19T23:29:24Z

tools/clang_tools/api_booster/main.cc

      // 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?


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] )

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Add missing legit word hackery Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

wrowe · 2019-12-20T18:42:19Z

/retest

repokitteh-read-only · 2019-12-20T18:42:22Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #9399 (comment) was created by @wrowe.

see: more, trace.

wrowe · 2019-12-20T18:44:46Z

The OS/X failure to grab bazelisk-darwin-amd64 is unrelated to this commit, can you re-review this PR please, @zuercher?

zuercher

Thanks!

This corrects the newest breakage on the Windows platform introduced in December. Risk Level: Low Testing: Local on Windows msvc and Linux gcc Docs Changes: n/a Release Notes: n/a Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

wrowe and others added 3 commits December 18, 2019 11:32

ifdef within a macro results in undefined behavior

3d8ef7a

- gcc and clang may handle this but msvc does not - see section 16.3/11 of the standard Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

Add spelling words, correct non-words

5e313e8

This restores master to passing the validation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

jmarantz previously approved these changes Dec 19, 2019

View reviewed changes

jmarantz assigned zuercher Dec 19, 2019

Remove reduncies as suggested by jmarantz

1a46407

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

sunjayBhatia dismissed jmarantz’s stale review via 1a46407 December 19, 2019 18:53

zuercher reviewed Dec 19, 2019

View reviewed changes

wrowe added 3 commits December 20, 2019 11:50

Merge branch 'master' into fix-win32-breakage-18-dec

0b4daa3

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Drop misspellings as suggested by zuercher

90f7117

Add missing legit word hackery Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Simplify as suggested by zuercher

5baa115

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

zuercher approved these changes Dec 20, 2019

View reviewed changes

zuercher merged commit 411a953 into envoyproxy:master Dec 20, 2019

wrowe deleted the fix-win32-breakage-18-dec branch December 20, 2019 20:04

sunjayBhatia mentioned this pull request Dec 20, 2019

NT support #129

Closed

Conversation

wrowe commented Dec 18, 2019

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Dec 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wrowe commented Dec 20, 2019

Uh oh!

repokitteh-read-only bot commented Dec 20, 2019

Uh oh!

wrowe commented Dec 20, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants