Most of the class definition in llvm/include/llvm/Support/FileCheck.h
are actually implementation details that should not be relied upon. This
commit moves all of it in a new header file under
llvm/lib/Support/FileCheck. It also takes advantage of the code movement
to put the code into a new llvm::filecheck namespace.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 38755 Build 38754: arc lint + arc unit
Event Timeline
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
24 | Does this need a new namespace? We don't really use too many of them in LLVM & if things are moving out of the header, I'd expect this change would only reduce the chance of naming collisions and reduce the need for an extra namespace. | |
llvm/lib/Support/FileCheck.cpp | ||
1120–1128 | Probably best to separate out API changes like this into a separate patch from the code movement. |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
142 | I think it works in practice with ctor+dtor but is undefined behavior according to C++14 section 17.6.4.8 paragraph 2.5 IIRC. | |
llvm/lib/Support/FileCheck.cpp | ||
1732 | I had errors at first because of PatternContext being incomplete but the standards does allow unique_ptr to have an incomplete type parameter so I'll try again. |
- Remove filecheck namespace
- Use std::unique_ptr rather than raw pointers
- Separate API change in separate patch
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
15–21 | Can any of these headers be removed now? Same question goes for the other header. | |
24 | I personally would prefer the namespace, as it allows simplifying the naming of the various structs everywhere within the FileCheck code, and therefore making the code more readable. I think I suggested it a while back, but it didn't happen at the time for some reason that I forget. | |
llvm/lib/Support/FileCheck.cpp | ||
17 | To avoid confusion and ambiguity between this and the main FileCheck header, I'd be inclined to call the internal one FileCheckImpl.h or something along those lines. Using duplicate header names confuses IDEs like Visual Studio (I already have an issue with, for example, the three versions of Object.h in the llvm-objcopy project). | |
llvm/lib/Support/FileCheck.h | ||
1 ↗ | (On Diff #222305) | This line probably needs updating. |
9 ↗ | (On Diff #222305) | This comment probably needs to be updated to say something about it being an internal header. |
llvm/unittests/Support/FileCheckTest.cpp | ||
10 | I don't really have a solution here, but I don't like this need for a relative include path. Are there any other examples of internal headers being used in unit tests like this? I'm guessing not, but probably simply because those headers aren't tested directly. |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
24 | For reference, a couple of examples of namespace usage within areas of LLVM I've worked in are object (for the Object library) and objcopy (for the llvm-objcopy code). |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
24 | Forgive my naive question but why is a namespace necessary to simplfy the naming of internal types? I feel only type in the public interface would benefit from a namespace but then I'd need to come up with a new name for the FileCheck class to avoid the weird llvm::filecheck::FileCheck type. |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
24 | Random related musing: If the types are really internal, why are they called "FileCheckSomeClassName"? Could they just be named "SomeClassName"? The namespace is more for the public interface, yes, sorry for the confusion. For the FileCheck class name in that case, I'd probably go with something like "Checker" or "Rnner" since it runs the file checking code. |
- Rename internal header to FileCheckImpl.h and fixup heading comment
- Remove unneeded include from the headers
Waiting until tomorrow noon (October 2nd ,12pm BST) before committing to allow people to comment.
llvm/unittests/Support/FileCheckTest.cpp | ||
---|---|---|
10 | TBH I didn't check at the time but yes there is 21 examples in llvm/unittests: llvm/unittests/CodeGen/DIEHashTest.cpp I think the right solution would be to have internal headers in a separate directory tree (e.g. include/llvm/internal or something like that) but it should be done as a separate patch. I am not volunteering but if I don't forget by then I'll consider it once the whole patchset is committed. |
llvm/unittests/Support/FileCheckTest.cpp | ||
---|---|---|
10 | Fair enough. Your suggestion sounds reasonable to me. I might suggest "include/llvm/detail" as that is something I've seen used in other projects (e.g. Boost). |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
17 | I think the order here (FileCheckImpl.h second) is preferable. Putting the public header first ensures that it has no missing dependencies; then the Impl header next, because it's local, and it can depend on the public header. I think that matches the intent of the style guide. I don't see many examples where a module has both a public and a private header, but there are a few: lib/IR/Attributes.cpp, lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp. These both have the public header first. | |
llvm/lib/Support/FileCheckImpl.h | ||
16 | FILECHECKIMPL_H |
- Fix guard macro name for FileCheck internal header
- Fix name of internal header file in its heading comment
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
24 | The FileCheckStrings and FileCheckPattern have a FileCheck prefix from when the FileCheck checking code got turned into a library to avoid name conflict. Types added later on just kept with the pattern for the same reason. I agree that renaming is in order now, and I'll schedule a patch for that. For now I'd like to focus on the support for matching format (D60389) but promise to not add the FileCheck prefix in new types in the meantimes. As to the public interface I'm not convinced a namespace is warranted. There would be only 1 header file using it, 3 types and 2 cpp files. It feels overkill. | |
llvm/lib/Support/FileCheck.cpp | ||
17 | Yes, it's unaesthetic but it makes more sense to put the two FileCheck headers together and the implementation must come in second since IIRW it uses some type declared in the public header | |
llvm/unittests/Support/FileCheckTest.cpp | ||
10 | Thanks for the suggestion, will keep that in mind if I get around to do it. |
Can any of these headers be removed now? Same question goes for the other header.