yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
================
Comment at: clang/lib/Sema/Sema.cpp:1508
void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+ auto DiagsCountIt = DiagsCount.find(FD);
FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();
----------------
rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > It makes me a little uncomfortable to be holding an iterator this long
> > > while calling a fair amount of other stuff in the meantime.
> > >
> > > Your use of DiagsCount is subtle enough that it really needs to be
> > > explained in some comments. You're doing stuff conditionally based on
> > > both whether the entry exists but also whether it's non-zero.
> > added comments
> Okay, thanks for that. Two points, then. First, it looks like the count is
> really just a boolean for whether the function recursively triggers any
> diagnostics. And second, can't it just be as simple as whether we've
> visited that function at all in a context that's forcing diagnostics to be
> emitted? The logic seems to be to try to emit the diagnostics for each
> use-path, but why would we want that?
For the second comment, we need to visit a function again for each use-path
because we need to report each use-path that triggers a diagnostic, otherwise
users will see a new error after they fix one error, instead of seeing all the
errors at once.
For the first comment, I will change the count to two flags: one for the case
where the function is not in device context, the other is for the case where
the function is in device context. This will allow us to avoid redundant visits
whether or not we are in a device context.
================
Comment at: clang/lib/Sema/Sema.cpp:1553
if (!Caller)
ShouldEmit = IsKnownEmitted;
if ((!ShouldEmit && !S.getLangOpts().OpenMP && !Caller) ||
----------------
rjmccall wrote:
> This becomes global state for the visitor; that doesn't seem like it can be
> right.
This state is for the root node of each traversal initiated from the items in
DeclsToCheckForDeferredDiags. It only needs to be set once. Will rename it as
ShouldEmitRootNode and move it to checkRecordedDecl.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77028/new/
https://reviews.llvm.org/D77028
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
