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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:28
 namespace llvm {
+namespace filecheck {
 
----------------
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.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:16-25
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SourceMgr.h"
----------------
Can any of these headers be removed now? Same question goes for the other header.


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


================
Comment at: llvm/lib/Support/FileCheck.h:1
 //==-- llvm/Support/FileCheck.h ---------------------------*- C++ -*-==//
 //
----------------
This line probably needs updating.


================
Comment at: llvm/lib/Support/FileCheck.h:9
 //
 /// \file This file has some utilities to use FileCheck as an API
 //
----------------
This comment probably needs to be updated to say something about it being an internal header.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:10
 #include "llvm/Support/FileCheck.h"
+#include "../lib/Support/FileCheck.h"
 #include "gtest/gtest.h"
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67649