[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:94
+def note_fe_backend_in : Note<"In function '%0'">;
+def note_fe_backend_inlined : Note<"\twhich inlined function '%0'">;
 
----------------
erichkeane wrote:
> This tab in the diagnostic is odd, we never do this, we just count on the cascading notes to be clear.  Also, this probably needs bikeshedding for diagnostic messages.
I've removed the tab, and changed this to what you've suggested below. Still open for bikeshedding, but marking this thread as done.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:860
+
+  Diags.Report(diag::note_fe_backend_in) << llvm::demangle(D.getCaller().str());
+
----------------
nickdesaulniers wrote:
> erichkeane wrote:
> > Could we instead just make `demangle` take a `string_view` here?  It takes it by const-ref, which shows that it doesn't really seem to need it to be a string, so I would imagine this would be a minor refactor (to add such an overload).
> Do you mean `StringRef` rather than `std::string_view`?  The progammer's manual provides guidance on the former (https://llvm.org/docs/ProgrammersManual.html#the-stringref-class) but not that latter.
> 
> Otherwise is this what you had in mind?
> ```
> diff --git a/llvm/include/llvm/Demangle/Demangle.h b/llvm/include/llvm/Demangle/Demangle.h
> index 6133d0b95bbf..c47222f883e9 100644
> --- a/llvm/include/llvm/Demangle/Demangle.h
> +++ b/llvm/include/llvm/Demangle/Demangle.h
> @@ -11,6 +11,7 @@
>  
>  #include <cstddef>
>  #include <string>
> +#include <string_view>
>  
>  namespace llvm {
>  /// This is a llvm local version of __cxa_demangle. Other than the name and
> @@ -68,7 +69,7 @@ char *dlangDemangle(const char *MangledName);
>  /// \param MangledName - reference to string to demangle.
>  /// \returns - the demangled string, or a copy of the input string if no
>  /// demangling occurred.
> -std::string demangle(const std::string &MangledName);
> +std::string demangle(const std::string_view MangledName);
>  
>  bool nonMicrosoftDemangle(const char *MangledName, std::string &Result);
>  
> diff --git a/llvm/lib/Demangle/Demangle.cpp b/llvm/lib/Demangle/Demangle.cpp
> index 9d128424cabf..408112b9248e 100644
> --- a/llvm/lib/Demangle/Demangle.cpp
> +++ b/llvm/lib/Demangle/Demangle.cpp
> @@ -26,9 +26,10 @@ static bool isDLangEncoding(const std::string &MangledName) {
>           MangledName[1] == 'D';
>  }
>  
> -std::string llvm::demangle(const std::string &MangledName) {
> +std::string llvm::demangle(const std::string_view MangledName) {
>    std::string Result;
> -  const char *S = MangledName.c_str();
> +  std::string Mangled(MangledName);
> +  const char *S = Mangled.c_str();
>  
>    if (nonMicrosoftDemangle(S, Result))
>      return Result;
> @@ -43,7 +44,7 @@ std::string llvm::demangle(const std::string &MangledName) {
>      return Result;
>    }
>  
> -  return MangledName;
> +  return Mangled;
>  }
>  
>  bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
> ```
That yak shave is going on in https://reviews.llvm.org/D148566; in particular, it will require changes to libcxxabi, since llvm/Demangle is just copied from libcxxabi as the upstream source. As such, marking this thread done.

I'm happy to help clean up such crufty APIs, but we should not block this feature on that.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141451