Page MenuHomePhabricator

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

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



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).


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

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?


what is the + for ?

sammccall marked 2 inline comments as done.Nov 14 2019, 8:25 AM
sammccall added inline comments.

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.


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!


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

Hi @sammccall .
Just a heads up. Looks like this might have caused: .

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.