PiotrZSL planned changes to this revision.
PiotrZSL marked 4 inline comments as done.
PiotrZSL added a comment.
TODO: Resolve rest of review comments.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:14
void ExceptionAnalyzer::ExceptionInfo::registerException(
- const Type *ExceptionType) {
+ const Type *ExceptionType, const SourceLocation Loc) {
assert(ExceptionType != nullptr && "Only valid types are accepted");
----------------
isuckatcs wrote:
> Is there any particular reason for taking `SourceLocation` by value? A `const
> SourceLocation &` would avoid an unnecessary copy.
SourceLocation is 4 bytes in size (encoded as enum). In clang is always passed
by value,
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:21
void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
- const Throwables &Exceptions) {
- if (Exceptions.size() == 0)
+ const Throwables &Exceptions, const SourceLocation Loc) {
+ if (Exceptions.empty())
----------------
isuckatcs wrote:
> Same question here regarding the argument type.
Same answer, 4 bytes in size.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:26
+ for (const auto [ThrowType, ThrowLoc] : Exceptions)
+ ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
}
----------------
isuckatcs wrote:
> There shouldn't be any invalid location in the container. An exception is
> thrown by a statement, so we know where in source that happens.
yes and no, this is an safety... and we may still not see an "throw" if we call
function that explicitly declare that throw some exceptions, but we do not have
definition.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:428
+ (IgnoredTypes.count(TD->getName()) > 0)) {
+ It = ThrownExceptions.erase(It);
+ continue;
----------------
isuckatcs wrote:
> You are erasing something from a container on which you're iteration over.
> Are you sure the iterators are not invalidated, when the internal
> representation changes?
`erase` returns new valid iterator
https://en.cppreference.com/w/cpp/container/map/erase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153298/new/
https://reviews.llvm.org/D153298
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
