Move definitions to prevent incomplete types.

[massberg]

Summary

C++20 is more strict when erroring out due to incomplete types.
Thus the code required some restructoring so that it complies in C++20.

[ilya-biryukov]

There is potentially a way to move less code by keeping all declarations at place and only moving bodies of definitions of constructors and destructors to the .cpp file.
Not sure what's preferable (less code moves vs more functions inline in the header), @kadircet do you have an opinion?

[kadircet]

In D141671#4050928, @ilya-biryukov wrote:

There is potentially a way to move less code by keeping all declarations at place and only moving bodies of definitions of constructors and destructors to the .cpp file.
Not sure what's preferable (less code moves vs more functions inline in the header), @kadircet do you have an opinion?

Having as few code in headers as possible is the general style guide in LLVM, so I'd rather err towards that and put definitions out-of-line as much as possible. Therefore if we can getaway by just moving definitions out-of-line, while keeping rest of the header ordering the same, let's go with that if it's feasible

[massberg]

Having as few code in headers as possible is the general style guide in LLVM, so I'd rather err towards that and put definitions out-of-line as much as possible. Therefore if we can getaway by just moving definitions out-of-line, while keeping rest of the header ordering the same, let's go with that if it's feasible

Thanks! I will check if I can get a version without reordering.

[massberg]

Clean up code and only mode definitions.

[massberg]

I have updated the code. It turned out that much less code has to be moved than I initially thought. :)

[kadircet]

thanks, LGTM! (btw, i know it's too late already, but in theory clangd has a code action called move definition out-of-line, could help with such refactorings in the future, if you didn't know already)

[ilya-biryukov]

LGTM! Looks much cleaner now, BTW.

clang-tools-extra/clang-doc/Representation.h

 
 // A representation of a parsed comment.
 struct CommentInfo {
   CommentInfo() = default;
   CommentInfo(CommentInfo &Other) = delete;
   CommentInfo(CommentInfo &&Other) = default;
   CommentInfo &operator=(CommentInfo &&Other) = default;
 
-  bool operator==(const CommentInfo &Other) const {
-    auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
-                            SelfClosing, Explicit, AttrKeys, AttrValues, Args);
-    auto SecondCI =
-        std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
-                 Other.ParamName, Other.CloseName, Other.SelfClosing,
-                 Other.Explicit, Other.AttrKeys, Other.AttrValues, Other.Args);
-
-    if (FirstCI != SecondCI || Children.size() != Other.Children.size())
-      return false;
-
-    return std::equal(Children.begin(), Children.end(), Other.Children.begin(),
-                      llvm::deref<std::equal_to<>>{});
-  }
+  bool operator==(const CommentInfo &Other) const;
 
   // This operator is used to sort a vector of CommentInfos.
   // No specific order (attributes more important than others) is required. Any
   // sort is enough, the order is only needed to call std::unique after sorting
   // the vector.
-  bool operator<(const CommentInfo &Other) const {
-    auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
-                            SelfClosing, Explicit, AttrKeys, AttrValues, Args);
-    auto SecondCI =
-        std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
-                 Other.ParamName, Other.CloseName, Other.SelfClosing,
-                 Other.Explicit, Other.AttrKeys, Other.AttrValues, Other.Args);
-
-    if (FirstCI < SecondCI)
-      return true;
-
-    if (FirstCI == SecondCI) {
-      return std::lexicographical_compare(
-          Children.begin(), Children.end(), Other.Children.begin(),
-          Other.Children.end(), llvm::deref<std::less<>>());
-    }
-
-    return false;
-  }
+  bool operator<(const CommentInfo &Other) const;
 
   SmallString<16>
       Kind; // Kind of comment (FullComment, ParagraphComment, TextComment,
             // InlineCommandComment, HTMLStartTagComment, HTMLEndTagComment,
             // BlockCommandComment, ParamCommandComment,
             // TParamCommandComment, VerbatimBlockComment,
             // VerbatimBlockLineComment, VerbatimLineComment).
   SmallString<64> Text;      // Text of the comment.
struct Info {
   // Returns a reference to the parent scope (that is, the immediate parent
   // namespace or class in which this decl resides).
   llvm::Expected<Reference> getEnclosingScope();
 };
 
 // Info for namespaces.
 struct NamespaceInfo : public Info {
   NamespaceInfo(SymbolID USR = SymbolID(), StringRef Name = StringRef(),
-                StringRef Path = StringRef())
-      : Info(InfoType::IT_namespace, USR, Name, Path) {}
+                StringRef Path = StringRef());
 
   void merge(NamespaceInfo &&I);
 
   ScopeChildren Children;
 };
 
 // Info for symbols.
 struct SymbolInfo : public Info {
struct FunctionInfo : public SymbolInfo {
   std::optional<TemplateInfo> Template;
 };
 
 // TODO: Expand to allow for documenting templating, inheritance access,
 // friend classes
 // Info for types.
 struct RecordInfo : public SymbolInfo {
   RecordInfo(SymbolID USR = SymbolID(), StringRef Name = StringRef(),
-             StringRef Path = StringRef())
-      : SymbolInfo(InfoType::IT_record, USR, Name, Path) {}
+             StringRef Path = StringRef());
 
   void merge(RecordInfo &&I);
 
   // Type of this record (struct, class, union, interface).
   TagTypeKind TagType = TagTypeKind::TTK_Struct;
 
   // Full qualified name of this record, including namespaces and template
   // specializations.
struct TypedefInfo : public SymbolInfo {
   // Inidicates if this is a new C++ "using"-style typedef:
   //   using MyVector = std::vector<int>
   // False means it's a C-style typedef:
   //   typedef std::vector<int> MyVector;
   bool IsUsing = false;
 };
 
 struct BaseRecordInfo : public RecordInfo {
-  BaseRecordInfo() : RecordInfo() {}
+  BaseRecordInfo();
   BaseRecordInfo(SymbolID USR, StringRef Name, StringRef Path, bool IsVirtual,
-                 AccessSpecifier Access, bool IsParent)
-      : RecordInfo(USR, Name, Path), IsVirtual(IsVirtual), Access(Access),
-        IsParent(IsParent) {}
+                 AccessSpecifier Access, bool IsParent);
 
   // Indicates if base corresponds to a virtual inheritance
   bool IsVirtual = false;
   // Access level associated with this inherited info (public, protected,
   // private).
   AccessSpecifier Access = AccessSpecifier::AS_public;
   bool IsParent = false; // Indicates if this base is a direct parent
 };

clang-tools-extra/clang-doc/Representation.cpp

mergeInfos(std::vector<std::unique_ptr<Info>> &Values) {
   case InfoType::IT_typedef:
     return reduce<TypedefInfo>(Values);
   default:
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "unexpected info type");
   }
 }
 
+bool CommentInfo::operator==(const CommentInfo &Other) const {
+  auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
+                          SelfClosing, Explicit, AttrKeys, AttrValues, Args);
+  auto SecondCI =
+      std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
+               Other.ParamName, Other.CloseName, Other.SelfClosing,
+               Other.Explicit, Other.AttrKeys, Other.AttrValues, Other.Args);
+
+  if (FirstCI != SecondCI || Children.size() != Other.Children.size())
+    return false;
+
+  return std::equal(Children.begin(), Children.end(), Other.Children.begin(),
+                    llvm::deref<std::equal_to<>>{});
+}
+
+bool CommentInfo::operator<(const CommentInfo &Other) const {
+  auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
+                          SelfClosing, Explicit, AttrKeys, AttrValues, Args);
+  auto SecondCI =
+      std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
+               Other.ParamName, Other.CloseName, Other.SelfClosing,
+               Other.Explicit, Other.AttrKeys, Other.AttrValues, Other.Args);
+
+  if (FirstCI < SecondCI)
+    return true;
+
+  if (FirstCI == SecondCI) {
+    return std::lexicographical_compare(
+        Children.begin(), Children.end(), Other.Children.begin(),
+        Other.Children.end(), llvm::deref<std::less<>>());
+  }
+
+  return false;
+}
+
 static llvm::SmallString<64>
 calculateRelativeFilePath(const InfoType &Type, const StringRef &Path,
                           const StringRef &Name, const StringRef &CurrentPath) {
   llvm::SmallString<64> FilePath;
 
   if (CurrentPath != Path) {
     // iterate back to the top
     for (llvm::sys::path::const_iterator I =
void SymbolInfo::merge(SymbolInfo &&Other) {
   // Unconditionally extend the list of locations, since we want all of them.
   std::move(Other.Loc.begin(), Other.Loc.end(), std::back_inserter(Loc));
   llvm::sort(Loc);
   auto Last = std::unique(Loc.begin(), Loc.end());
   Loc.erase(Last, Loc.end());
   mergeBase(std::move(Other));
 }
 
+NamespaceInfo::NamespaceInfo(SymbolID USR, StringRef Name, StringRef Path)
+      : Info(InfoType::IT_namespace, USR, Name, Path) {}
+
 void NamespaceInfo::merge(NamespaceInfo &&Other) {
   assert(mergeable(Other));
   // Reduce children if necessary.
   reduceChildren(Children.Namespaces, std::move(Other.Children.Namespaces));
   reduceChildren(Children.Records, std::move(Other.Children.Records));
   reduceChildren(Children.Functions, std::move(Other.Children.Functions));
   reduceChildren(Children.Enums, std::move(Other.Children.Enums));
   reduceChildren(Children.Typedefs, std::move(Other.Children.Typedefs));
   mergeBase(std::move(Other));
 }
 
+RecordInfo::RecordInfo(SymbolID USR, StringRef Name, StringRef Path)
+    : SymbolInfo(InfoType::IT_record, USR, Name, Path) {}
+
 void RecordInfo::merge(RecordInfo &&Other) {
   assert(mergeable(Other));
   if (!TagType)
     TagType = Other.TagType;
   IsTypeDef = IsTypeDef || Other.IsTypeDef;
   if (Members.empty())
     Members = std::move(Other.Members);
   if (Bases.empty())
void TypedefInfo::merge(TypedefInfo &&Other) {
   assert(mergeable(Other));
   if (!IsUsing)
     IsUsing = Other.IsUsing;
   if (Underlying.Type.Name == "")
     Underlying = Other.Underlying;
   SymbolInfo::merge(std::move(Other));
 }
 
+BaseRecordInfo::BaseRecordInfo() : RecordInfo() {}
+
+BaseRecordInfo::BaseRecordInfo(SymbolID USR, StringRef Name, StringRef Path,
+                               bool IsVirtual, AccessSpecifier Access,
+                               bool IsParent)
+    : RecordInfo(USR, Name, Path), IsVirtual(IsVirtual), Access(Access),
+      IsParent(IsParent) {}
+
 llvm::SmallString<16> Info::extractName() const {
   if (!Name.empty())
     return Name;
 
   switch (IT) {
   case InfoType::IT_namespace:
     // Cover the case where the project contains a base namespace called
     // 'GlobalNamespace' (i.e. a namespace at the same level as the global