Page MenuHomePhabricator

[clang-tidy] refactor ExceptionAnalyzer further to give ternary answer
ClosedPublic

Authored by JonasToth on Feb 7 2019, 3:40 AM.

Details

Summary

The analsis on the throwing behvaiour on functions and statements gave only
a binary answer whether an exception could occur and if yes which types are
thrown.
This refactoring allows keeping track if there is a unknown factor, because the
code calls to some functions with unavailable source code with no noexcept
information.
This 'potential Unknown' information is propagated properly and can be queried
separately.

Diff Detail

Event Timeline

JonasToth created this revision.Feb 7 2019, 3:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 3:40 AM

Exciting!

clang-tidy/utils/ExceptionAnalyzer.h
21–24

Would it be better to add them into ExceptionAnalyzer class?

145

Why can't llvm::DenseMap continue to be used?

JonasToth marked 2 inline comments as done.Feb 7 2019, 7:33 AM
JonasToth added inline comments.
clang-tidy/utils/ExceptionAnalyzer.h
21–24

ThrownExceptions? I think it fits here. Removing would shrink ExceptionInfo (and ContainsUnknown and State could be packed into one byte) which is a plus.

145

I would need to add traits for ExceptionInfo to work with DenseMap. So i went this way :)
From the docs DenseMap shall map small types (like ptr-ptr) but the ExceptionInfo has the set of types as SmallSet<Type*, 8>, so not sure if that is actually a good fit.

lebedev.ri added inline comments.Feb 7 2019, 9:12 AM
clang-tidy/utils/ExceptionAnalyzer.h
21–24

No, i meant all these classes/enums.

class ExceptionAnalyzer {
public:
enum class ExceptionState : std::int8_t {
....
}


class ExceptionInfo {
....
}

} // class ExceptionAnalyzer
JonasToth updated this revision to Diff 185939.Feb 8 2019, 3:44 AM
  • [Refactor] move support classes into the analyzer
  • [Refactor] move bigger methods into implementation file
JonasToth marked 2 inline comments as done.Feb 8 2019, 3:44 AM

Good idea :)

JonasToth marked an inline comment as done.Feb 8 2019, 3:48 AM

If I understand it correctly, this is more of an infrastructure improvement than check enhancement. It looks like a nice and clean code. Where do we expect to use this new behavior? In the current check or in the upcoming "modernize" check?

clang-tidy/utils/ExceptionAnalyzer.h
128

Why 4 bits, not just 2? Can we expect additional states?

lebedev.ri accepted this revision.Feb 19 2019, 2:16 AM

Sorry for the reviews, i'm really stalling it seems..

This looks like a straight-forward change, but i'm not sure i'm familiar enough with ExceptionAnalyzer to fully properly review this..
As far as i can tell, this is ok. Perhaps @baloghadamsoftware will have other remarks.

If I understand it correctly, this is more of an infrastructure improvement than check enhancement. It looks like a nice and clean code. Where do we expect to use this new behavior? In the current check or in the upcoming "modernize" check?

It's for D57108, i'we guessed that such ternary answer will be required there.

clang-tidy/utils/ExceptionAnalyzer.h
26–31

Since this is later used in a bit field, it might be better to be explicit

enum class State : std::int8_t {
  Throwing = 0,    ///< The function can definitly throw given an AST.
  NotThrowing = 1, ///< This function can not throw, given an AST.
  Unknown = 2,     ///< This can happen for extern functions without available
               ///< definition.
};

and indeed, only 2 bits needed.

145

Good point.

This revision is now accepted and ready to land.Feb 19 2019, 2:16 AM

It's for D57108, i'we guessed that such ternary answer will be required there.

Oh, I was not aware of that patch.

Anyway, it looks good to me.

JonasToth marked 2 inline comments as done.
  • [Refactor] move support classes into the analyzer
  • [Refactor] move bigger methods into implementation file
  • minor adjustments
JonasToth marked 3 inline comments as done.Feb 20 2019, 11:34 AM

all comments resolved. I will land this now.

clang-tidy/utils/ExceptionAnalyzer.h
26–31

Done.

128

True, math not so strong in me :D

JonasToth marked an inline comment as done.
  • be explicit about the State enumerator values
This revision was automatically updated to reflect the committed changes.
bjope added inline comments.
clang-tidy/utils/ExceptionAnalyzer.h
112

(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.

Maybe @sammccall remembers why it was decided to rewrite the enum into constexpr:s in https://reviews.llvm.org/rCTE319608 ? Do we need a similar solution here?

JonasToth marked an inline comment as done.Feb 26 2019, 1:55 AM
JonasToth added inline comments.
clang-tidy/utils/ExceptionAnalyzer.h
112

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.

JonasToth marked an inline comment as done.Feb 26 2019, 1:57 AM
Charusso removed a subscriber: Charusso.Feb 26 2019, 2:00 AM
bjope added inline comments.Feb 27 2019, 2:06 AM
clang-tidy/utils/ExceptionAnalyzer.h
112

( 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!