[PATCH] D65778: [FileCheck] Add missing includes in header

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked an inline comment as done.
thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:21
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/Error.h"
----------------
grimar wrote:
> It's unsorted. Should probably be:
> 
> ```
> #include "llvm/ADT/Optional.h"
> #include "llvm/ADT/StringMap.h"
> #include "llvm/ADT/StringSet.h"
> ```
> 
> (In LLD we always sort headers, I am not sure this rule applies to whole LLVM, but I think it is).
> 
> But first of all I am unsure we want to include these headers after `"llvm/Support/FileCheck.h"` where
> they are already included. Do we really want it? Why?
It's debatable. My thoughts were to include it if there's any use of a type that doesn't come from implementing an interface declared in FileCheck.h. So if interfaces change to no longer use StringRef for instance, headers inclusion only change in FileCheck.h and FileCheck.cpp still compiles (after changing the prototype of course) despite there still being some StringRef.

But I did wonder for a while whether FileCheck.cpp needed some includes as well and am happy to not change the include there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65778