cleaned up FwdAnalysis - #8652
Conversation
Sorry, something went wrong.
| struct Result { | ||
| enum class Type : std::uint8_t { NONE, READ, WRITE, BREAK, RETURN, BAILOUT } type; | ||
| explicit Result(Type type) : type(type) {} | ||
| Result(Type type, const Token *token) : type(type), token(token) {} | ||
| const Token* token{}; | ||
| }; | ||
|
|
||
| enum class What : std::uint8_t { Reassign, UnusedValue }; |
There was a problem hiding this comment.
Maybe I missed it. But I feel these should be in some namespace to avoid possible ODR violations.
| struct Result { | |
| enum class Type : std::uint8_t { NONE, READ, WRITE, BREAK, RETURN, BAILOUT } type; | |
| explicit Result(Type type) : type(type) {} | |
| Result(Type type, const Token *token) : type(type), token(token) {} | |
| const Token* token{}; | |
| }; | |
| enum class What : std::uint8_t { Reassign, UnusedValue }; | |
| namespace { | |
| struct Result { | |
| enum class Type : std::uint8_t { NONE, READ, WRITE, BREAK, RETURN, BAILOUT } type; | |
| explicit Result(Type type) : type(type) {} | |
| Result(Type type, const Token *token) : type(type), token(token) {} | |
| const Token* token{}; | |
| }; | |
| enum class What : std::uint8_t { Reassign, UnusedValue }; | |
| } |
Sorry, something went wrong.
There was a problem hiding this comment.
I am not sure that is necessary as clang-tidy should complain about that.
Sorry, something went wrong.
There was a problem hiding this comment.
according to AI, clang-tidy has these checkers for ODR:
- misc-definitions-in-headers: Flags variables and non-inline functions defined in headers, which will cause ODR violations if included in multiple translation units.
- misc-anonymous-namespace-in-header: Warns about anonymous namespaces in headers, which creates unique definitions in every translation unit.
- bugprone-suspicious-include: Detects if you include implementation files (like .cpp files), which commonly leads to ODR violations.
none of those seem to check this code properly.
Sorry, something went wrong.
There was a problem hiding this comment.
As usual AI doesn't have any clue...
The actual check is misc-use-internal-linkage. But AnalyzeTypes is currently disabled because it is too intrusive. If this is desired I can enable that in a follow-up.
Sorry, something went wrong.
There was a problem hiding this comment.
So if we turn on that warning then there will be a warning here. I would like that we fix it directly. And turning on that warning later in a follow-up sounds good.
Sorry, something went wrong.
There was a problem hiding this comment.
because it is too intrusive
hmm.. I would like to have anonymous namespaces in sourcefiles wrapping types.
the types in our header files should ideally be wrapped in some namespace but imho it's too intrusive to do that for now. does clang-tidy require this?
Sorry, something went wrong.
No description provided.