NagyDonat wrote: > > This commit uses `setDeclWithIssue` in a way that is not consistent with > > the intended meaning and other usage of this method. Its (admittedly a bit > > vague, but still understandable) doc-comment says that its role is > > ```c++ > > // Specifically set the Decl where an issue occurred. This isn't necessary > > // for BugReports that cover a path as it will be automatically inferred. > > ``` > > This is true for PathSensitive issues.
@steakhal Note that I copied the quoted comment from the method of the non-path-sensitive `BugReport` class. > However, webkit checkers piggyback on the CSA infra but only use the > `runCheckersOnASTDecl` parts. This is where is gets tricky because there is > no good decl CSA could set the `DeclWithIssue` to. Yes, the case of reports generated from `runCheckersOnASTDecl` is troublesome because there we don't have a good automatic solution. For path-sensitive reports "This [=calling `setDeclWithIssue`] isn't necessary" because there the analyzer can automatically set a good `DeclWithIssue`. On the other hand, for non-path-sensitive reports (reports that do not "cover a path"), I think the unsaid implication of the doc-comment is that `setDeclWithIssue` must be called manually because (as you nicely explain) the engine cannot select a good `Decl` automatically. > [...] For any other Decl. This is where we should set the `DeclWithIssue` to > the decl we visit. This is not done. This will likely use the TUDecl it set > when visiting the TUDecl. This is correct, but likely we should set the Decl > we pass to the callback. I also suspect that it would be probably better to set `DeclWithIssue` to a more concrete decl when we visit it. If we think that this is important, an experienced contributor should inspect the implications of this (what kinds of `Decl`s can appear instead of the TUDecl, are we prepared to handle those, how does this tweak impact the result set on real-world projects etc.). As the status quo seems to be mostly stable, I'm not convinced that this is worth the required effort (there is a code smell, but nothing is on fire, unlike in other areas of the analyzer). > So ultimately checkers have 2 options: [... use the `DeclWithIssue` from the > callback or pick their own `Decl` manually] I agree, and I agree that these two options are valid. > So all in all, I don't see any problem here. @steakhal The problem (which is already fixed by @ziqingluo-90) was that the original version of the PR (which was swept away by a force push) passed "small" decls (fields, local variables etc.) to `setDeclWithIssue` and extended `GetEnclosingDeclContextSignature` with the counter-intuitive behavior that when `DeclWithIssue` is-a such "small" desc, then the hash will include the qualified name [or signature] of the _decl that encloses `DeclWithIssue`_ (which breaks the usual pattern that the hash includes the qualname or signature of `DeclWithIssue` itself). https://github.com/llvm/llvm-project/pull/214102 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
