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

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe 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;
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/safety-no-vector-bool.cpp:37
+std::vector<bool, user_allocator<bool>> v4;  
+
----------------
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.


https://reviews.llvm.org/D29118