[PATCH] D29118: [clang-tidy] safety-no-vector-bool

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
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:
> 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 ;)


================
Comment at: clang-tools-extra/test/clang-tidy/safety-no-vector-bool.cpp:37
+std::vector<bool, user_allocator<bool>> v4;  
+
----------------
jbcoe wrote:
> JonasToth wrote:
> > what happens for types where std::vector<bool> would be an template argument? for example std::pair and tuple could contain a vector<bool>.
> > is there a warning as well?
> Nicely spotted. Those won't get picked up right now and need to be. 
> 
> I'm struggling to build a matcher for this. We really need to find any place where `std::vector<bool>` is used as a template argument.
i found hasAnyTemplateArgument in the ast matcher refrence. did u use that one?


https://reviews.llvm.org/D29118