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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
