[include-cleaner] Add the missing parts of Symbol and Header clases.

[sammccall]

Add comment on variant/enum order match.

[kadircet]

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h

49

nit: how about implementing this with std::visit? it's definitely more code, but at least makes sure:

• we won't depend on the order in Kind matching the Storage, and
• we'd get a hard compiler error when a new variant is introduced without proper handling here

it would look like:

Kind kind() const {
  struct KindTranslator {
      Kind operator()(Decl *) { return Declaration; }
      ...
  };
  return std::visit(KindTranslator{}, Storage);
}

but not feeling so strongly about it, so feel free to keep as is if needed (I don't think we'll add tons of more kinds later on, so the chances of introducing bugs is small. but using new language/library constructs sounds fun).
(same for Header)

55

nit: I think it would be better to spell out the types here (and above), rather than rely on what Kind would translate into

61

nit: any particular reason for dropping const here?

[sammccall]

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h

49

This is a bunch more code to read, is no longer a trivial accessor when compiled. It does look canonical, but I think this speaks more to std::variant being not great (we don't have a lot of experience with it yet).

It's not *that* much more safe, the error enum {A, C, B} can still be expressed as

Kind operator()(A*)  { return A; }
Kind operator()(B*)  { return A; /*oops*/ }

It's silghtly more locally obvious, but it's buried in a bunch of boilerplate.

(It also leans into addressing variants with types, as I mentioned below)

55

This seems backwards to me: the compiler will check that the type is correct, but can't check that the alternative we're fetching matches the accessor we're defining.
This version places declaration() and Declaration next to each other so such a mistake is obvious.

(As a design question, I think we should to name variants rather than access them with types when this is ergonomic, e.g. it's perfectly reasonable to model header=physical/verbatim/stdlib as variant<FileEntry*, StringRef, StringRef>. Languages that take variants seriously tend to prefer this)

61

Oh, the constructor was nonconst so I assumed const was an oversight. Changed to const everywhere.

[kadircet]

thanks, i agree with your comments around std::variant, let's see what kind of patterns will emerge in the future.

so, still LG, but please address the triple slash issue and const-ness before landing

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h

40

triple slashes (here and elsewhere)

61

i guess you forgot to upload the diff? (the constructor was definitely an oversight, thanks for noticing that)

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h

 
 #ifndef CLANG_INCLUDE_CLEANER_TYPES_H
 #define CLANG_INCLUDE_CLEANER_TYPES_H
 
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
 #include <memory>
 #include <vector>
 
+namespace llvm {
+class raw_ostream;
+} // namespace llvm
 namespace clang {
 class Decl;
 class FileEntry;
 namespace include_cleaner {
 
 /// An entity that can be referenced in the code.
 struct Symbol {
-  Symbol(Decl &D) : Storage(&D) {}
+  enum Kind {
+    /// A canonical clang declaration.
+    Declaration,
+    /// A recognized symbol from the standard library, like std::string.
+    Standard,
+  };
+
+  Symbol(const Decl &D) : Storage(&D) {}
   Symbol(tooling::stdlib::Symbol S) : Storage(S) {}
 
+  Kind kind() const { return static_cast<Kind>(Storage.index()); }
+  bool operator==(const Symbol &RHS) const { return Storage == RHS.Storage; }
+
+  tooling::stdlib::Symbol standard() const {
+    return std::get<Standard>(Storage);
+  }
+  const Decl &declaration() const { return *std::get<Declaration>(Storage); }
+
 private:
   // FIXME: Add support for macros.
+  // Order must match Kind enum!
   std::variant<const Decl *, tooling::stdlib::Symbol> Storage;
 };
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Symbol &);
 
 /// Represents a file that provides some symbol. Might not be includeable, e.g.
 /// built-in or main-file itself.
 struct Header {
-  /// A physical (or logical, in case of a builtin) file.
+  enum Kind {
+    /// A source file parsed by clang. (May also be a <built-in> buffer).
+    Physical,
+    /// A recognized standard library header, like <string>.
+    Standard,
+  };
+
   Header(const FileEntry *FE) : Storage(FE) {}
-  /// A logical file representing a stdlib header.
   Header(tooling::stdlib::Header H) : Storage(H) {}
 
+  Kind kind() const { return static_cast<Kind>(Storage.index()); }
   bool operator==(const Header &RHS) const { return Storage == RHS.Storage; }
 
+  const FileEntry *physical() const { return std::get<Physical>(Storage); }
+  tooling::stdlib::Header standard() const {
+    return std::get<Standard>(Storage);
+  }
+
 private:
   // FIXME: Handle verbatim spellings.
+  // Order must match Kind enum!
   std::variant<const FileEntry *, tooling::stdlib::Header> Storage;
 };
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Header &);
 
 } // namespace include_cleaner
 } // namespace clang
 
 #endif
 

clang-tools-extra/include-cleaner/lib/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangIncludeCleaner
   Analysis.cpp
   HTMLReport.cpp
   Record.cpp
+  Types.cpp
   WalkAST.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangLex
   clangToolingInclusionsStdlib
   )
 

clang-tools-extra/include-cleaner/lib/Types.cpp

+//===--- Types.cpp --------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang-include-cleaner/Types.h"
+#include "clang/AST/Decl.h"
+#include "clang/Basic/FileEntry.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang::include_cleaner {
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S) {
+  switch (S.kind()) {
+  case Symbol::Declaration:
+    if (const auto *ND = llvm::dyn_cast<NamedDecl>(&S.declaration()))
+      return OS << ND->getNameAsString();
+    return OS << S.declaration().getDeclKindName();
+  case Symbol::Standard:
+    return OS << S.standard().scope() << S.standard().name();
+  }
+  llvm_unreachable("Unhandled Symbol kind");
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Header &H) {
+  switch (H.kind()) {
+  case Header::Physical:
+    return OS << H.physical()->getName();
+  case Header::Standard:
+    return OS << H.standard().name();
+  }
+  llvm_unreachable("Unhandled Header kind");
+}
+
+} // namespace clang::include_cleaner