[PATCH] D67649: [FileCheck] Move private interface to its own header

Tue Oct 1 13:35:00 PDT 2019

thopre added inline comments.

================
Comment at: llvm/include/llvm/Support/FileCheck.h:28
 namespace llvm {
+namespace filecheck {

----------------
jhenderson wrote:
> thopre wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > dblaikie wrote:
> > > > > 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.
> > > > 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.
> > > 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).
> > 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.
> 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.
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. 

================
Comment at: llvm/lib/Support/FileCheck.cpp:17
 #include "llvm/Support/FileCheck.h"
+#include "FileCheckImpl.h"
 #include "llvm/ADT/StringSet.h"
----------------
probinson wrote:
> grimar wrote:
> > 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.
> 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.
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

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:10
 #include "llvm/Support/FileCheck.h"
+#include "../lib/Support/FileCheck.h"
 #include "gtest/gtest.h"
----------------
jhenderson wrote:
> thopre wrote:
> > jhenderson wrote:
> > > 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.
> > 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.
> 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).
Thanks for the suggestion, will keep that in mind if I get around to do it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67649