Page MenuHomePhabricator

Refactoring the attribute plugin example to fit the new API
ClosedPublic

Authored by psionic12 on Nov 23 2020, 11:13 PM.

Details

Summary

Make the example compiled and the test case passed.

Diff Detail

Event Timeline

psionic12 created this revision.Nov 23 2020, 11:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 11:13 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
psionic12 requested review of this revision.Nov 23 2020, 11:13 PM
aaron.ballman added inline comments.Dec 4 2020, 10:55 AM
clang/examples/Attribute/Attribute.cpp
26–29

The comment is a bit misleading, how about: Can take up to 15 optional arguments, to emulate accepting a variadic number of arguments.?

59–69

This comment describes what the code is doing but not why. How about: The example attribute supports up to 15 optional arguments, but this demonstrates how to test for a specific number of arguments. or something along those lines?

67

If has arguments -> If there are arguments

(And add a full stop to the end of the comment.)

92

This looks dangerous to me -- if there are two or three arguments supplied, then this code will result in a buffer overrun.

clang/test/Frontend/plugin-attribute.cpp
1–3

I don't think we need a GOOD_ATTR and BAD_ATTR run line any longer. Instead, you can use a single RUN line with -verify (no =) and use expected-error and expected-warning on the lines that need the diagnostics. This means you can also get rid of the @ line number constructs and just write the expected comments on the source lines with diagnostics.

12

A downside to this is that we lose the verification that the plugin actually attaches the attribute to the AST node. Another solution that doesn't involve generating LLVM bitcode from a Frontend test would be to use -ast-dump and check the AST nodes.

psionic12 updated this revision to Diff 309813.Dec 6 2020, 11:29 PM

Add tests to check if attributes are attached to AST

psionic12 updated this revision to Diff 309814.Dec 6 2020, 11:32 PM
psionic12 marked an inline comment as done.

Fix grammar

psionic12 marked 4 inline comments as done.Dec 7 2020, 1:14 AM
psionic12 added inline comments.
clang/examples/Attribute/Attribute.cpp
92

Oh, didn't noticed that, but I checked the code it's hard to get the address of the argument buffer, all APIs about it is private, even if ArgsUnion const *getArgsBuffer() const, which I think is not very reasonable. Do you think I should copy all the arguments to a new buffer (not very effective), or do you think I should contribute a patch to make the API public?

There's a third way, getTrailingObjects() is public in TrailingObjects, it is possible to cast an ParsedAttr to a TrailingObjects, but I think it's too hacky.

clang/test/Frontend/plugin-attribute.cpp
1–3

Seems the GOOD_ATTR and BAD_ATTR need to be exist since we use -ast-dump now, we need a clean build without error diagnostics to generate a nice AST.

psionic12 updated this revision to Diff 309853.Dec 7 2020, 3:08 AM
psionic12 marked 2 inline comments as done.

Use a vector to collect arguments

psionic12 added inline comments.Dec 7 2020, 3:10 AM
clang/examples/Attribute/Attribute.cpp
92

I take times trying and found out it's more difficult to use the original buffer, so I use a vector to collect the arguments instead.

psionic12 marked 2 inline comments as not done.Dec 7 2020, 3:11 AM
aaron.ballman added inline comments.Dec 8 2020, 9:57 AM
clang/examples/Attribute/Attribute.cpp
65

only allowed -> accepts

77

How about: first argument to the 'example' attribute must be a string literal

clang/test/Frontend/plugin-attribute.cpp
1–3

That suggests this should be two test files with one RUN line apiece.

psionic12 updated this revision to Diff 310401.Dec 8 2020, 6:19 PM

Fix grammar
Simplify the test case

aaron.ballman added inline comments.Dec 9 2020, 11:21 AM
clang/test/Frontend/plugin-attribute.cpp
1

I am unfamiliar with this construct -- is there a reason to do this as opposed to making two separate files with separate content (these are testing fundamentally different things)?

psionic12 marked 5 inline comments as done.Dec 9 2020, 6:03 PM
psionic12 added inline comments.
clang/test/Frontend/plugin-attribute.cpp
1

I just followed the suggestion from this, the document suggest that if the testing files are small, considered to put them in a single file, and use the split-file command. I think the testing infrastructure treat this as two files when testing.

aaron.ballman accepted this revision.Dec 10 2020, 4:58 AM

Thanks for the changes, LGTM!

clang/test/Frontend/plugin-attribute.cpp
1

Huh... we have exactly one other test case using this construct, which explains why I've never heard of it. :-D Thank you for pointing out that documentation.

I think it's usually best to keep test files focused on what they're testing, and this tests two different things (AST node attachment and semantic diagnostics) so my natural inclination is to have two separate files. But this provides the same test coverage, so I'd say we can leave it as-is until there's some other reason to split it up.

This revision is now accepted and ready to land.Dec 10 2020, 4:58 AM
keryell retitled this revision from Refactoring the attrubute plugin example to fit the new API to Refactoring the attribute plugin example to fit the new API.Dec 10 2020, 10:25 PM
psionic12 marked an inline comment as done.Dec 20 2020, 7:55 PM

Hi, could anyone help to commit this?

aaron.ballman closed this revision.Dec 21 2020, 5:25 AM

Hi, could anyone help to commit this?

I'm happy to do so, sorry about not asking earlier. I've commit on your behalf in b2ba6867eac10874bd279c739639bdb9e60c1996.

The last update didn't update the expected errors. I have done so in 9a93f95fce91fb4616cee0f307b564b253789282 and it's now passing. (I think it missed the bots since they don't build examples)

The last update didn't update the expected errors. I have done so in 9a93f95fce91fb4616cee0f307b564b253789282 and it's now passing. (I think it missed the bots since they don't build examples)

Oh, thank you for this! I'm guessing my local build also missed this for the same reason.