[PATCH] D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer

[Bjorn Pettersson via Phabricator via cfe-commits]

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