JonasToth marked an inline comment as done. JonasToth added inline comments.
================ Comment at: clang-tidy/utils/ExceptionAnalyzer.h:112 + /// throw, if it's unknown or if it won't throw. + enum State Behaviour : 2; + ---------------- bjope wrote: > (post-commit comments) > > I've seen that @dyung did some VS2015 fixes related to this in > https://reviews.llvm.org/rCTE354545. > > But I think there also is some history of problems with typed enums used in > bitfields with GCC (I'm not sure about the details, but it might be discussed > here https://reviews.llvm.org/D24289, and there have been workarounds applied > before, for example here https://reviews.llvm.org/rCTE319608). > > I do not know if we actually have a goal to workaround such problems (no > warnings when using GCC), nor do I know if GCC produce correct code despite > all the warnings we see with GCC (7.4.0) after this patch, such as > > ``` > ../tools/clang/tools/extra/clang-tidy/bugprone/../utils/ExceptionAnalyzer.h:112:23: > warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' > is too small to hold all values of 'enum class > clang::tidy::utils::ExceptionAnalyzer::State' > State Behaviour : 2; > ^ > ../tools/clang/tools/extra/clang-tidy/bugprone/../utils/ExceptionAnalyzer.h:112:23: > warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' > is too small to hold all values of 'enum class > clang::tidy::utils::ExceptionAnalyzer::State' > State Behaviour : 2; > ^ > In file included from > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:9:0: > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.h:112:23: > warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' is > too small to hold all values of 'enum class > clang::tidy::utils::ExceptionAnalyzer::State' > State Behaviour : 2; > ^ > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp: In member > function 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo& > clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::merge(const > clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo&)': > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:40:28: > warning: comparison is always false due to limited range of data type > [-Wtype-limits] > else if (Other.Behaviour == State::Unknown && Behaviour == > State::NotThrowing) > ~~~~~~~~~~~~~~~~^~~~~~~~ > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:41:24: > warning: overflow in implicit constant conversion [-Woverflow] > Behaviour = State::Unknown; > ^~~~~~~ > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp: In member > function 'void > clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::reevaluateBehaviour()': > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:105:26: > warning: overflow in implicit constant conversion [-Woverflow] > Behaviour = State::Unknown; > ^~~~~~~ > ``` > > To sum up: > The warning are a little bit annoying (but maybe something we have to live > with if this is known bugs in gcc). > It seems like we have done other workarounds in the past (writing ugly code > to satisfy MSVC and GCC). So should we do that here as well? > Might wanna check if GCC produce correct code for this. I see. All i wanted to achieve was to make the object <= 64 bytes to fit in a cacheline. IMHO the bitfields can disapear. I dont want to sacrifice the `enum class` as I feel that gives more value then the (unmeasured) potential performance gain from shrinking the object sizes. I will commit to remove the bitfields. Details can be figured out later. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57883/new/ https://reviews.llvm.org/D57883 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
