jvoung wrote:

> > > we're not (fully) understanding the content
> > 
> > 
> > My thinking was that we don't even need to understand the content, we 
> > simply exclude code that is contained within any of the problematic public 
> > macros. This sounds like it should be possible to do? Unfortunately I don't 
> > know the details on how this could be implemented, hopefully other 
> > reviewers know better?
> > Otherwise ChatGPT seems to give useful ideas on how to skip a matched 
> > result contained within an `ASSERT` macro (obviously untested):
> > ```
> >   if (Lexer::getImmediateMacroName(Loc, SM, Result.Context->getLangOpts()) 
> > == "ASSERT") {
> >     // The call is within ASSERT, no diagnostic needed.
> >     return;
> >   }
> > ```
> 
> That doesn't handle some cases like:
> 
> ```
> auto opt = DoSomeSetup(...)
> ASSERT_TRUE(opt.has_value())
> T x = DoMoreSetup(*opt)  // warn right here, since we didn't interpret the 
> above ASSERT_TRUE (or other ways to check)
> 
> EXPECT_EQ(FunctionToTest(x), ...);
> ```
> 
> Sometimes the `*opt` may be within a macro, but not always.

- In non-test code, it is even more likely that the `*opt` is not wrapped in a 
macro, while the `ASSERT(opt.has_value())` is.
- If in non-test scenarios, the `ASSERT` macro implementation is actually 
simple, and the analysis already understood `ASSERT(opt.has_value())` makes a 
following `*opt` safe, then ignoring the `ASSERT` is actually worse and causes 
false positives.
- We wouldn't want to accidentally ignore the wrong macros (especially in 
non-test contexts)
- Coming up with a list of exactly the right macros to ignore for gtest, would 
be a bigger list of public macro names than the current change.
- And it still doesn't solve the "sometimes the `*opt` may be within a macro, 
but not always."

https://github.com/llvm/llvm-project/pull/115051
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to