aaron.ballman added a subscriber: cjdb.
aaron.ballman added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:863
+  InliningDecisions.push_back(D.getCaller().str());
+  for (size_t i = 0, e = InliningDecisions.size(); i != e; ++i) {
+    std::string S = llvm::demangle(InliningDecisions[i]);
----------------
Alternatively:
```
for (auto Dec : llvm::enumerate(InliningDecisions)) {
  std::string S = llvm::demangle(Dec.value());
  if (Dec.index() == 0)
    Diags.Report(...);
  ...
}
```


================
Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:12
   foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh 
no foo}}
+         // expected-note@* {{In function 'baz'}}
   if (x())
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > Instead of allowing this note to appear anywhere in the file, I think 
> > > it's better to use "bookmark" comments. e.g.,
> > > ```
> > > void baz() { // #baz_defn
> > > }
> > > 
> > > void foo() {
> > >   baz(); // expected-note@#baz_defn {{whatever note text}}
> > > }
> > > ```
> > > so that we're testing where the diagnostic is emitted. (This is 
> > > especially important given that the changes are about location tracking.)
> > oh, that's new (to me)! will do
> It looks like the "bookmarks" don't work because the notes do not line+col 
> info. They follow the warning/error diagnostic which does, on the bottom most 
> call site.
> 
> The warning is supposed to be emitted on the callsite, not the definition.
I still don't think this is reasonable for test coverage because this means 
we'll accept the note *anywhere* in the file. This makes it much too easy to 
regress the behavior accidentally (e.g., accidentally emit all the notes on 
line 1 of the file). I think we need *some* way to nail down where these notes 
are emitted in the test. However, I might be asking you to solve "please track 
location information through the backend" for that, so perhaps this isn't a 
reasonable request?

Also, CC @cjdb for awareness of how this kind of diagnostic is produced (Chris 
is working on an idea for how we emit diagnostics so we get better structured 
information from them, so knowing about this novel approach might impact that 
idea).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141451/new/

https://reviews.llvm.org/D141451

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to