Page MenuHomePhabricator

[AST] Attach comment in `/** doc */ typedef struct A {} B` to B as well as A.
ClosedPublic

Authored by sammccall on Nov 13 2019, 12:30 PM.

Details

Summary

Semantically they're the same thing, and it's important when the underlying
struct is anonymous.

There doesn't seem to be a problem attaching the same comment to multiple things
as it already happens with /** doc */ int a, b;

This affects an Index test but the results look better (name present, USR points
to the typedef).

Fixes https://github.com/clangd/clangd/issues/189

Diff Detail

Event Timeline

sammccall created this revision.Nov 13 2019, 12:30 PM

Build result: pass - 59972 tests passed, 0 failed and 763 were skipped.
Log files: console-log.txt, CMakeCache.txt

kadircet added inline comments.Nov 13 2019, 11:59 PM
clang/test/Index/annotate-comments-typedef.m
20

sorry for being dense, what exactly is the change here ? it looks like comment was already being attributed to MyEnum.
As for the USR I agree that this looks a whole lot better, apparently for anon decls USR contains the name of the typedef decl instead.

What is the extra text that we don't care in between?

clang/test/Sema/warn-documentation.cpp
871

what is the + for ?

sammccall marked 2 inline comments as done.Nov 14 2019, 8:25 AM
sammccall added inline comments.
clang/test/Index/annotate-comments-typedef.m
20

Yeah, most of this doesn't matter AFAIK and the test is just brittle, as lit tests are wont to be.

The {{.*}} is because the output now includes the RawComment= clause, that previously wasn't included. (Which I think is to do with the comment being immediately before the location?)

Previously the Name was <anonymous>, and now it's MyEnum. And the USR has changed as you noted.

clang/test/Sema/warn-documentation.cpp
871

The warning is now emitted twice, once for the embedded struct definition and once for the typedef.
This isn't perfect but I don't think it's worth fixing. (Again, it's the existing behavior for a broken comment before int a,b,c)

kadircet accepted this revision.Mon, Nov 18, 1:33 AM
kadircet marked an inline comment as done.

LGTM, thanks!

clang/test/Index/annotate-comments-typedef.m
20

oh missed the change in Name thanks for pointing it out!

This revision is now accepted and ready to land.Mon, Nov 18, 1:33 AM
This revision was automatically updated to reflect the committed changes.
bjope added a subscriber: bjope.Tue, Nov 26, 8:26 AM

Hi @sammccall .
Just a heads up. Looks like this might have caused: https://bugs.llvm.org/show_bug.cgi?id=44143 .

Hi @sammccall .
Just a heads up. Looks like this might have caused: https://bugs.llvm.org/show_bug.cgi?id=44143 .

Yes indeed. I locally reverted commit a433e714 and it resolved my issue. It's odd, though. The unittest Sam added is nearly identical with the reproducer I posted. Only obvious difference I see at first blush is that my reproducer uses the same identifier for the struct name and the typedef name, whereas Sam's unittest uses a unique identifier for each. Semantically, there is no difference, though.