jbcoe marked 5 inline comments as done.
jbcoe added a comment.
Handling code like `std::pair<std::vector<bool>, int>` is currently eluding me.
If I define `std::pair` to have members `first` and `second` then
`std::pair<std::vector<bool>, int>` triggers the `vector<bool>` diagnostic but
in the template, not the instantiation point.
I need to match class definitions that have `vector<bool>` as a template
argument and exclude them from later matchers. I can't see how to do this right
now. I suspect some more sophisticated matchers or approaches are needed.
================
Comment at: clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp:50
+ diag(MatchedDecl->getLocation(),
+ " function %0 returns an instance of std::vector<bool>")
+ << MatchedDecl;
----------------
JonasToth wrote:
> jbcoe wrote:
> > djehuti wrote:
> > > JonasToth wrote:
> > > > i think all those diag() calls can be merged into one. inside the
> > > > if/else-if you can just set a StringRef with the specific part of the
> > > > warning, and have a parameterized diag() at the end of the function.
> > > >
> > > > in NoMallocCheck there is a similar pattern:
> > > >
> > > > const CallExpr *Call = nullptr;
> > > >
> > > > StringRef Recommendation;
> > > >
> > > >
> > > >
> > > > if ((Call = Result.Nodes.getNodeAs<CallExpr>("aquisition")))
> > > >
> > > > Recommendation = "consider a container or a smart pointer";
> > > >
> > > > else if ((Call = Result.Nodes.getNodeAs<CallExpr>("realloc")))
> > > >
> > > > Recommendation = "consider std::vector or std::string";
> > > >
> > > > else if ((Call = Result.Nodes.getNodeAs<CallExpr>("free")))
> > > >
> > > > Recommendation = "use RAII";
> > > >
> > > >
> > > >
> > > > assert(Call && "Unhandled binding in the Matcher");
> > > >
> > > >
> > > >
> > > > diag(Call->getLocStart(), "do not manage memory manually; %0")
> > > >
> > > > << Recommendation << SourceRange(Call->getLocStart(),
> > > > Call->getLocEnd());
> > > >
> > > Except with braces, right? (That's another High-Integrity C++ rule btw.)
> > > ;)
> > I agree that this _can_ be done but I'm not convinced it helps readability.
> > Repetition is partial and very localized. I'll happily make the change if
> > you feel strongly that it's an improvement.
> i think either is ok. maybe someone else prefers one strongly over the other,
> but i dont mind.
>
> but i think the else path should exist, make an failing assert or sth like
> that, for the safety ;)
Agreed on the else, especially as it agrees with the safety-critical standards
we're checking.
================
Comment at: clang-tools-extra/clang-tidy/safety/NoVectorBoolCheck.cpp:50
+ diag(MatchedDecl->getLocation(),
+ " function %0 returns an instance of std::vector<bool>")
+ << MatchedDecl;
----------------
jbcoe wrote:
> JonasToth wrote:
> > jbcoe wrote:
> > > djehuti wrote:
> > > > JonasToth wrote:
> > > > > i think all those diag() calls can be merged into one. inside the
> > > > > if/else-if you can just set a StringRef with the specific part of the
> > > > > warning, and have a parameterized diag() at the end of the function.
> > > > >
> > > > > in NoMallocCheck there is a similar pattern:
> > > > >
> > > > > const CallExpr *Call = nullptr;
> > > > >
> > > > > StringRef Recommendation;
> > > > >
> > > > >
> > > > >
> > > > > if ((Call = Result.Nodes.getNodeAs<CallExpr>("aquisition")))
> > > > >
> > > > > Recommendation = "consider a container or a smart pointer";
> > > > >
> > > > > else if ((Call = Result.Nodes.getNodeAs<CallExpr>("realloc")))
> > > > >
> > > > > Recommendation = "consider std::vector or std::string";
> > > > >
> > > > > else if ((Call = Result.Nodes.getNodeAs<CallExpr>("free")))
> > > > >
> > > > > Recommendation = "use RAII";
> > > > >
> > > > >
> > > > >
> > > > > assert(Call && "Unhandled binding in the Matcher");
> > > > >
> > > > >
> > > > >
> > > > > diag(Call->getLocStart(), "do not manage memory manually; %0")
> > > > >
> > > > > << Recommendation << SourceRange(Call->getLocStart(),
> > > > > Call->getLocEnd());
> > > > >
> > > > Except with braces, right? (That's another High-Integrity C++ rule
> > > > btw.) ;)
> > > I agree that this _can_ be done but I'm not convinced it helps
> > > readability. Repetition is partial and very localized. I'll happily make
> > > the change if you feel strongly that it's an improvement.
> > i think either is ok. maybe someone else prefers one strongly over the
> > other, but i dont mind.
> >
> > but i think the else path should exist, make an failing assert or sth like
> > that, for the safety ;)
> Agreed on the else, especially as it agrees with the safety-critical
> standards we're checking.
LLVM/Clang tends not to use braces for single statement control flow, but maybe
we should follow safety-critical rules in a safety-critical check?
https://reviews.llvm.org/D29118
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
