dblaikie marked an inline comment as done. dblaikie added a comment. In D102122#3550590 <https://reviews.llvm.org/D102122#3550590>, @aaron.ballman wrote:
> In D102122#3544655 <https://reviews.llvm.org/D102122#3544655>, @dblaikie > wrote: > >> How's this look now, then? > > I think it's heading in the right direction. > >> One drawback is the built-in attribute warnings that mention which entities >> the attribute is valid on will mention typedefs even when the user uses a >> spelling that wouldn't be valid there - so the user might try it there, then >> get the follow-on warning (totally open to wordsmithing that warning, too - >> just placeholder words - also made it part of the same group as the >> warn_unused_result warning itself - if you disable that warning, then >> presumably you don't care about the warning being in the wrong place either, >> to some extent). > > Yeah, that is a drawback -- we don't have a spelling-specific set of subjects > currently. I had a plan to address that at some point by introducing more > data into the subject list so you could do something like: > > def WarnUnusedResult : InheritableAttr { > let Spellings = [CXX11<"", "nodiscard", 201907>, > C2x<"", "nodiscard", 201904>, > CXX11<"clang", "warn_unused_result">, > GCC<"warn_unused_result">]; > let Subjects = SubjectList< > SubjectsFor<[ObjCMethod, Enum, Record, > FunctionLike, TypedefName], [GNU<"warn_unused_result">, CXX11<"clang", > "warn_unused_result">]>, > SubjectsFor<[ObjCMethod, Enum, Record, > FunctionLike], [CXX11<"", "nodiscard">, C2x<"", "nodiscard">, CXX11<"gnu", > "warn_unused_result">]>, > >; > ... > } > > But.... that's complicated (as this attribute demonstrates!). This is a case > where we want HALF of a spelling to apply to some subjects (the GNU spelling > half of the `GCC` spelling) and the other half to apply to other subjects > (the C++ spelling half of the `GCC` spelling). > > I think for right now, I think it's okay for us to have the slightly degraded > diagnostic behavior. If it turns into user confusion in practice, we can > improve the diagnostics at that point. WDYT? Oh, yeah, should've said I'm OK with it if you are. Only meant to point it out to get confirmation there. Thanks! ================ Comment at: clang/include/clang/Basic/Attr.td:2945 C2x<"", "nodiscard", 201904>, CXX11<"clang", "warn_unused_result">, GCC<"warn_unused_result">]; ---------------- aaron.ballman wrote: > Oops, it looks like we support this spelling in C++ but not in C (not your > bug, just an observation): https://godbolt.org/z/1hPq6coba Oh, so we'd have to put `C2x<"clang", "warn_unused_result">` in here? Fair enough, yeah, separate issue. ================ Comment at: clang/lib/AST/Expr.cpp:1525-1527 + if (const auto *TD = getCallReturnType(Ctx)->getAs<TypedefType>()) + if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>()) + return A; ---------------- aaron.ballman wrote: > rsmith wrote: > > This should loop over `TypedefType` sugar; the attribute could be in a > > nested typedef. > I agree with Richard here. I think this is needed for a case like: > ``` > typedef int foo __attribute__((warn_unused_result)); > typedef foo foob; > > foob func(); > > int main() { > func(); // Still want to be barked at here > } > ``` > But this example brings up an interesting question of expectations. What's > your intuition on how this should behave? > ``` > typedef int foo __attribute__((warn_unused_result)); > typedef foo *foo_ptr; > > foo_ptr func(); > > int main() { > func(); // Bark? Silence? Other? > } > ``` > FWIW, my initial inclination was that this would diagnose the call to > `func()` but now I'm second-guessing that, hence the question about type > composition in a typedef. > I agree with Richard here. I think this is needed for a case like: > ``` > typedef int foo __attribute__((warn_unused_result)); > typedef foo foob; > > foob func(); > > int main() { > func(); // Still want to be barked at here > } > ``` Oh, sorry, I marked this as "done" because I'd done this - but hadn't updated the patch. Should be updated now, including a test case (warn-unused-result.cpp:unused_typedef_result::indirect). > But this example brings up an interesting question of expectations. What's > your intuition on how this should behave? > ``` > typedef int foo __attribute__((warn_unused_result)); > typedef foo *foo_ptr; > > foo_ptr func(); > > int main() { > func(); // Bark? Silence? Other? > } > ``` > FWIW, my initial inclination was that this would diagnose the call to func() > but now I'm second-guessing that, hence the question about type composition > in a typedef. Yeah, I'm OK with/think that probably shouldn't warn - like instantiating a template with a warn_unused_result doesn't/shouldn't designate that template specialization as warn_unused_result - it's a related/derived type, but doesn't necessarily have the same properties (I guess another example in that direction: a function pointer, where one of the parameters/result type are warn_unused_result doesn't make the function pointer type itself warn_unused_result). ================ Comment at: clang/test/SemaCXX/warn-unused-result.cpp:259 +namespace unused_typedef_result { +[[clang::warn_unused_result]] typedef void *a; +a af1(); ---------------- aaron.ballman wrote: > I think you should also have a test for `[[gnu::warn_unused_result]]` to show > that we didn't touch the behavior of that vendor-specific spelling. Ah, sure - done! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102122/new/ https://reviews.llvm.org/D102122 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
