nickdesaulniers marked an inline comment as done. nickdesaulniers added inline comments.
================ Comment at: clang/lib/CodeGen/CodeGenAction.cpp:860 + + Diags.Report(diag::note_fe_backend_in) << llvm::demangle(D.getCaller().str()); + ---------------- 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) { ``` ================ 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()) ---------------- 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 ================ Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:29 + // expected-note@* {{In function 'd'}} + // expected-note@* {{which inlined function 'b'}} + // expected-note@* {{which inlined function 'a'}} ---------------- erichkeane wrote: > Same comment re-bookmarks, but this diagnostic seems awkward. > > Should we instead say : > > `call to 'foo' declared with 'error' attribute: oh no foo` > `called by function 'a'` > `inlined by function 'b'` > `inlined by function 'd'`? > ah, yeah, I have this backwards (in v1, I had fixed this in the alt impl https://reviews.llvm.org/D145994). Let me fix that up here now, too. Good catch! ================ Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:2467-2468 + + const Function *Callee = CI->getCalledFunction(); + if (Callee && (Callee->hasFnAttribute("dontcall-error") || + Callee->hasFnAttribute("dontcall-warn"))) { ---------------- nickdesaulniers wrote: > nickdesaulniers wrote: > > nickdesaulniers wrote: > > > arsenm wrote: > > > > Misses constexpr casts and aliases > > > The base feature doesn't work with aliases (or ConstantExpr), in GCC or > > > Clang. I should perhaps fix that first... > > Perhaps I should use Call.getCalledOperand()->stripPointerCasts() for > > constantexpr case. > I've added support for ConstantExpr to the base feature in D142058. I should > still fix this in this PR and add a test case, so not yet marking this thread > as done. marking this thread as done; see other thread below. If someone adds an alias, maybe they _intend_ to call the warning/error attributed function for [[good reason]]. 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
