bjope 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; + ---------------- JonasToth wrote: > 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. ( regarding https://reviews.llvm.org/rGc1e8cbd5c3f0ed33ab4fb8df98645ebea0018fe8 ) Pity if we have to sacrifice performance if these are false warnings from gcc. But I don't really have the details about that, so thanks for the fix! 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
