This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Add missing includes in header
ClosedPublic

Authored by thopre on Aug 5 2019, 2:42 PM.

Event Timeline

thopre created this revision.Aug 5 2019, 2:42 PM
grimar added inline comments.Aug 6 2019, 12:40 AM
llvm/lib/Support/FileCheck.cpp
21

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?

thopre updated this revision to Diff 213530.Aug 6 2019, 1:02 AM
thopre marked an inline comment as done.

Address review comment

thopre marked an inline comment as done.Aug 6 2019, 1:07 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
21

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.

jhenderson added inline comments.Aug 6 2019, 2:10 AM
llvm/lib/Support/FileCheck.cpp
22

Have you read the relevant part of the coding standards? llvm/Support/FileCheck.h should be first as the file's own header. Additionally, clang-format sorts things appropriately, and the coding standards say you don't need to include headers if they're included by a referenced header.

thopre updated this revision to Diff 213553.Aug 6 2019, 2:40 AM
  • Include FileCheck.h first
  • Remove includes in FileCheck.cpp of headers already included in FileCheck.h
thopre marked an inline comment as done.Aug 6 2019, 2:40 AM
jhenderson accepted this revision.Aug 6 2019, 2:58 AM

LGTM, I think.

This revision is now accepted and ready to land.Aug 6 2019, 2:58 AM
grimar accepted this revision.Aug 6 2019, 2:59 AM

Looks good to me too.

This revision was automatically updated to reflect the committed changes.