[PATCH] D92006: Refactoring the attrubute plugin example to fit the new API

Thu Dec 10 04:58:57 PST 2020

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Thanks for the changes, LGTM!

================
Comment at: clang/test/Frontend/plugin-attribute.cpp:1
-// RUN: %clang -fplugin=%llvmshlibdir/Attribute%pluginext -emit-llvm -S %s -o - 2>&1 | FileCheck %s --check-prefix=ATTRIBUTE
-// RUN: not %clang -fplugin=%llvmshlibdir/Attribute%pluginext -emit-llvm -DBAD_ATTRIBUTE -S %s -o - 2>&1 | FileCheck %s --check-prefix=BADATTRIBUTE
+// RUN: split-file %s %t
+// RUN: %clang -cc1 -load %llvmshlibdir/Attribute%pluginext -fsyntax-only -ast-dump -verify %t/good_attr.cpp | FileCheck %s
----------------
psionic12 wrote:
> aaron.ballman wrote:
> > 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)?
> I just followed the suggestion from [[ https://llvm.org/docs/TestingGuide.html#extra-files | 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92006/new/

https://reviews.llvm.org/D92006