[PATCH] D80786: Rename APIs in unittests/AST/Language.h in preparation to share them

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri May 29 04:18:26 PDT 2020


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

Agree with the rationale about moving the names out of `clang::ast_matchers`, and the names not being safe to put into `clang::` as-is.

The names are a bit unwieldy though, and I wonder if all of them are patterns we want to encourage using widely (at least if so, the names become more important).

I'd consider:

- Unittest -> Test (shorter, and unit test is two words, and llvm naming conventions notwithstanding many gtests are not unit tests)
- moving into a clang::test namespace instead of prefixing the names
- UnitTestClangArgs --> `std::vector<std::string>` it's harder to parse but much more widely understood

Anyway, don't want to block this, this is an important step to sharing which is worthwhile whether the names are my favourite or not :-)



================
Comment at: clang/unittests/AST/Language.h:23
+
+enum UnittestLanguage {
+  Lang_C,
----------------
this enum doesn't seem great: why is the default CXX (presumably) 98? How do you get objc++ with 14 features?

(No need to address this now, but it's not clear this is a great API to spread more widely. Vs e.g.
```
struct TestLanguage {
  static TestLanguage CXX; // some arbitrary version
  static TestLanguage C;
  
  enum {
    CXX98,
    CXX11,
  } CXXVersion;

  vector<string> getCommandLineArgs();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80786





More information about the cfe-commits mailing list