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

Tue Oct 1 07:59:11 PDT 2019

probinson added inline comments.

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

================
Comment at: llvm/lib/Support/FileCheckImpl.h:16
+#ifndef LLVM_SUPPORT_FILECHECK_PRIVATE_H
+#define LLVM_SUPPORT_FILECHECK_PRIVATE_H

----------------
FILECHECKIMPL_H

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67649