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

[Jonas Toth via Phabricator via cfe-commits]

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