ci: enable check for static non-POD #5602
Conversation
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
There was a problem hiding this comment.
Having to NOLINT all of the factories is pretty annoying. Is there any way around this?
TBH I'm kind of blah on this, and I think our adherence to the static init fiasco rule is a bit draconian. Meaning, if something breaks, we can fix it, but I'm not sure we have to take it so seriously (I guess that's just me).
If we make the registration as macro, we might be able to put NOLINT in the macro. I'll give it a try. |
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
include/envoy/singleton/manager.h
Outdated
| @@ -45,7 +45,7 @@ template <const char* name_param> class RegistrationImpl : public Registration { | |||
| */ | |||
| #define SINGLETON_MANAGER_REGISTRATION(NAME) \ | |||
| static constexpr char NAME##_singleton_name[] = #NAME "_singleton"; \ | |||
| static Envoy::Registry::RegisterFactory< \ | |||
| static Envoy::Registry::RegisterFactory< /* NOLINT(fuchsia-statically-constructed-objects) */ \ | |||
There was a problem hiding this comment.
@mattklein123 we can do sth like this for extensions if we use macro to register, WDYT? cc @htuch
There was a problem hiding this comment.
I guess? In general this strikes me as solving a problem that doesn't actually need solving, but I guess if we insist on not having any non-POD statics in the code base we have to do ithis?
There was a problem hiding this comment.
I think let's switch to a macro here, we might find this useful in the future due to the increased freedom of implementation.
On the point of being too draconian, my main wish being fulfilled with this is to reduce review noise re: static init fiasco and to avoid having a human in the loop missing the bad static init fiasco cases. So, if we can do this without too much NOLINT noise I'm still in favor.
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
@lizan where are we with this PR? |
stale
bot
removed
the
stale
|
will update this shortly... |
*Description*: From discussion in #5602. *Risk Level*: Low *Testing*: CI *Docs Changes*: *Release Notes*: Signed-off-by: Lizan Zhou <lizan@tetrate.io>
|
@htuch PTAL, should be good to go. |
This reverts commit 3e5b286. Signed-off-by: Lizan Zhou <lizan@tetrate.io>
*Description*: From discussion in envoyproxy#5602. *Risk Level*: Low *Testing*: CI *Docs Changes*: *Release Notes*: Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Lizan Zhou <lizan.j@gmail.com>
Check global static non-POD in clang-tidy to avoid static initialization fiasco. https://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html Risk Level: Low Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Lizan Zhou <lizan.j@gmail.com>
…proxy#5733) This reverts commit 3e5b286. Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Lizan Zhou <lizan.j@gmail.com>
*Description*: From discussion in envoyproxy#5602. *Risk Level*: Low *Testing*: CI *Docs Changes*: *Release Notes*: Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Fred Douglas <fredlas@google.com>
Check global static non-POD in clang-tidy to avoid static initialization fiasco. https://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html Risk Level: Low Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Fred Douglas <fredlas@google.com>
…proxy#5733) This reverts commit 3e5b286. Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Lizan Zhou lizan@tetrate.io
Description:
Check global static non-POD in clang-tidy to avoid static initialization fiasco.
https://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html
Risk Level: Low
Testing:
Docs Changes:
Release Notes: