Page MenuHomePhabricator

[FileCheck] Move private interface to its own header
ClosedPublic

Authored by thopre on Sep 17 2019, 1:53 AM.

Details

Summary

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.

Event Timeline

thopre created this revision.Sep 17 2019, 1:53 AM
arichardson added inline comments.Sep 17 2019, 2:12 AM
llvm/include/llvm/Support/FileCheck.h
147

Why can't this be by-vaule? Shouldn't a vector of an incomplete type be fine as long as you have an out-of-line ctor+dtor?

llvm/lib/Support/FileCheck.cpp
1726

Use unique_ptr instead of new delete?

thopre updated this revision to Diff 220454.Sep 17 2019, 2:17 AM

Fix codestyle

dblaikie added inline comments.Sep 17 2019, 1:52 PM
llvm/include/llvm/Support/FileCheck.h
28

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
1121–1122

Probably best to separate out API changes like this into a separate patch from the code movement.

thopre marked 2 inline comments as done.Sep 18 2019, 9:07 AM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
147

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
1726

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.

thopre updated this revision to Diff 222305.Sep 28 2019, 3:54 PM
thopre marked 4 inline comments as done.
  • Remove filecheck namespace
  • Use std::unique_ptr rather than raw pointers
  • Separate API change in separate patch
jhenderson added inline comments.Sep 30 2019, 4:09 AM
llvm/include/llvm/Support/FileCheck.h
16–25

Can any of these headers be removed now? Same question goes for the other header.

28

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

This line probably needs updating.

9

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.

jhenderson added inline comments.Sep 30 2019, 4:28 AM
llvm/include/llvm/Support/FileCheck.h
28

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

thopre marked an inline comment as done.Sep 30 2019, 5:38 AM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
28

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.

jhenderson added inline comments.Sep 30 2019, 6:01 AM
llvm/include/llvm/Support/FileCheck.h
28

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.

thopre updated this revision to Diff 222433.Sep 30 2019, 7:54 AM
thopre marked 6 inline comments as done.
  • Rename internal header to FileCheckImpl.h and fixup heading comment
  • Remove unneeded include from the headers
jhenderson accepted this revision.Sep 30 2019, 9:57 AM

LGTM, but I wouldn't mind a second opinion.

This revision is now accepted and ready to land.Sep 30 2019, 9:57 AM
thopre added a comment.Oct 1 2019, 1:56 AM

LGTM, but I wouldn't mind a second opinion.

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
llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp
llvm/unittests/Transforms/Vectorize/VPlanDominatorTreeTest.cpp
llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp
llvm/unittests/Transforms/Vectorize/VPlanLoopInfoTest.cpp
llvm/unittests/Transforms/Vectorize/VPlanPredicatorTest.cpp
llvm/unittests/Transforms/Vectorize/VPlanSlpTest.cpp
llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
llvm/unittests/Transforms/Vectorize/VPlanTestBase.h
llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp
llvm/unittests/tools/llvm-cfi-verify/GraphBuilder.cpp
llvm/unittests/tools/llvm-exegesis/ARM/AssemblerTest.cpp
llvm/unittests/tools/llvm-exegesis/X86/AssemblerTest.cpp
llvm/unittests/tools/llvm-exegesis/X86/SnippetGeneratorTest.cpp
llvm/unittests/tools/llvm-exegesis/X86/SnippetRepetitorTest.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.

jhenderson added inline comments.Oct 1 2019, 2:03 AM
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).

grimar accepted this revision.Oct 1 2019, 2:05 AM

LGTM with 2 minor nits.

llvm/lib/Support/FileCheck.cpp
17

I'd reorder to either place FileCheckImpl.h before or after llvm/* includes group for consistency.
I believe this is a more common way in LLVM.

llvm/lib/Support/FileCheckImpl.h
1 ↗(On Diff #222433)

FileCheck.h -> FileCheckImpl.h

probinson added inline comments.Oct 1 2019, 7:58 AM
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 ↗(On Diff #222433)

FILECHECKIMPL_H

thopre updated this revision to Diff 222688.Oct 1 2019, 1:34 PM
thopre marked 11 inline comments as done.
  • Fix guard macro name for FileCheck internal header
  • Fix name of internal header file in its heading comment
llvm/include/llvm/Support/FileCheck.h
28

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.

This revision was automatically updated to reflect the committed changes.