[PATCH] D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer
Roman Lebedev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 19 02:16:33 PST 2019
lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.
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.
In D57883#1402023 <https://reviews.llvm.org/D57883#1402023>, @baloghadamsoftware wrote:
> 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 <https://reviews.llvm.org/D57108>, i'we guessed that such ternary answer will be required there.
================
Comment at: clang-tidy/utils/ExceptionAnalyzer.h:218
llvm::StringSet<> IgnoredExceptions;
- llvm::DenseMap<const FunctionDecl *, bool> FunctionCache;
+ std::map<const FunctionDecl *, ExceptionInfo> FunctionCache;
};
----------------
JonasToth wrote:
> lebedev.ri wrote:
> > Why can't `llvm::DenseMap` continue to be used?
> 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.
Good point.
================
Comment at: clang-tidy/utils/ExceptionAnalyzer.h:26-31
+ enum class State : std::int8_t {
+ Throwing, ///< The function can definitly throw given an AST.
+ NotThrowing, ///< This function can not throw, given an AST.
+ Unknown, ///< This can happen for extern functions without available
+ ///< definition.
+ };
----------------
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.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57883/new/
https://reviews.llvm.org/D57883
More information about the cfe-commits
mailing list