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

[Jonathan B Coe via Phabricator via cfe-commits]

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