Merge topic 'refactor-backtrace'

22aa6b67b4 cmListFileCache: Refactor cmListFileBacktrace internals
1fe0d72eb6 clang-tidy: exclude 'misc-noexcept-move-constructor'

Acked-by: Kitware Robot <[email protected]>
Merge-request: !2410

.clang-tidy

@@ -5,6 +5,7 @@ misc-*,\
 -misc-incorrect-roundings,\
 -misc-macro-parentheses,\
 -misc-misplaced-widening-cast,\
+-misc-noexcept-move-constructor,\
 -misc-static-assert,\
 modernize-*,\
 -modernize-deprecated-headers,\

Source/cmListFileCache.cxx

@@ -11,6 +11,7 @@
 
 #include <algorithm>
 #include <assert.h>
+#include <memory>
 #include <sstream>
 
 cmCommandContext::cmCommandName& cmCommandContext::cmCommandName::operator=(
@@ -285,91 +286,66 @@ bool cmListFileParser::AddArgument(cmListFileLexer_Token* token,
   return true;
 }
 
-struct cmListFileBacktrace::Entry : public cmListFileContext
+// We hold either the bottom scope of a directory or a call/file context.
+// Discriminate these cases via the parent pointer.
+struct cmListFileBacktrace::Entry
 {
-  Entry(cmListFileContext const& lfc, Entry* up)
-    : cmListFileContext(lfc)
-    , Up(up)
-    , RefCount(0)
+  Entry(cmStateSnapshot bottom)
+    : Bottom(bottom)
   {
-    if (this->Up) {
-      this->Up->Ref();
-    }
   }
-  ~Entry()
+
+  Entry(std::shared_ptr<Entry const> parent, cmListFileContext lfc)
+    : Context(std::move(lfc))
+    , Parent(std::move(parent))
   {
-    if (this->Up) {
-      this->Up->Unref();
-    }
   }
-  void Ref() { ++this->RefCount; }
-  void Unref()
+
+  ~Entry()
   {
-    if (--this->RefCount == 0) {
-      delete this;
+    if (this->Parent) {
+      this->Context.~cmListFileContext();
+    } else {
+      this->Bottom.~cmStateSnapshot();
     }
   }
-  Entry* Up;
-  unsigned int RefCount;
-};
 
-cmListFileBacktrace::cmListFileBacktrace(cmStateSnapshot const& bottom,
-                                         Entry* up,
-                                         cmListFileContext const& lfc)
-  : Bottom(bottom)
-  , Cur(new Entry(lfc, up))
-{
-  assert(this->Bottom.IsValid());
-  this->Cur->Ref();
-}
-
-cmListFileBacktrace::cmListFileBacktrace(cmStateSnapshot const& bottom,
-                                         Entry* cur)
-  : Bottom(bottom)
-  , Cur(cur)
-{
-  if (this->Cur) {
-    assert(this->Bottom.IsValid());
-    this->Cur->Ref();
-  }
-}
+  bool IsBottom() const { return !this->Parent; }
 
-cmListFileBacktrace::cmListFileBacktrace()
-  : Bottom()
-  , Cur(nullptr)
-{
-}
+  union
+  {
+    cmStateSnapshot Bottom;
+    cmListFileContext Context;
+  };
+  std::shared_ptr<Entry const> Parent;
+};
 
 cmListFileBacktrace::cmListFileBacktrace(cmStateSnapshot const& snapshot)
-  : Bottom(snapshot.GetCallStackBottom())
-  , Cur(nullptr)
+  : TopEntry(std::make_shared<Entry const>(snapshot.GetCallStackBottom()))
 {
 }
 
-cmListFileBacktrace::cmListFileBacktrace(cmListFileBacktrace const& r)
-  : Bottom(r.Bottom)
-  , Cur(r.Cur)
+cmListFileBacktrace::cmListFileBacktrace(std::shared_ptr<Entry const> parent,
+                                         cmListFileContext const& lfc)
+  : TopEntry(std::make_shared<Entry const>(std::move(parent), lfc))
 {
-  if (this->Cur) {
-    assert(this->Bottom.IsValid());
-    this->Cur->Ref();
-  }
 }
 
-cmListFileBacktrace& cmListFileBacktrace::operator=(
-  cmListFileBacktrace const& r)
+cmListFileBacktrace::cmListFileBacktrace(std::shared_ptr<Entry const> top)
+  : TopEntry(std::move(top))
 {
-  cmListFileBacktrace tmp(r);
-  std::swap(this->Cur, tmp.Cur);
-  std::swap(this->Bottom, tmp.Bottom);
-  return *this;
 }
 
-cmListFileBacktrace::~cmListFileBacktrace()
+cmStateSnapshot cmListFileBacktrace::GetBottom() const
 {
-  if (this->Cur) {
-    this->Cur->Unref();
+  cmStateSnapshot bottom;
+  if (Entry const* cur = this->TopEntry.get()) {
+    while (Entry const* parent = cur->Parent.get()) {
+      cur = parent;
+    }
+    bottom = cur->Bottom;
   }
+  return bottom;
 }
 
 cmListFileBacktrace cmListFileBacktrace::Push(std::string const& file) const
@@ -380,54 +356,61 @@ cmListFileBacktrace cmListFileBacktrace::Push(std::string const& file) const
   // skipped during call stack printing.
   cmListFileContext lfc;
   lfc.FilePath = file;
-  return cmListFileBacktrace(this->Bottom, this->Cur, lfc);
+  return this->Push(lfc);
 }
 
 cmListFileBacktrace cmListFileBacktrace::Push(
   cmListFileContext const& lfc) const
 {
-  return cmListFileBacktrace(this->Bottom, this->Cur, lfc);
+  assert(this->TopEntry);
+  assert(!this->TopEntry->IsBottom() || this->TopEntry->Bottom.IsValid());
+  return cmListFileBacktrace(this->TopEntry, lfc);
 }
 
 cmListFileBacktrace cmListFileBacktrace::Pop() const
 {
-  assert(this->Cur);
-  return cmListFileBacktrace(this->Bottom, this->Cur->Up);
+  assert(this->TopEntry);
+  assert(!this->TopEntry->IsBottom());
+  return cmListFileBacktrace(this->TopEntry->Parent);
 }
 
 cmListFileContext const& cmListFileBacktrace::Top() const
 {
-  if (this->Cur) {
-    return *this->Cur;
-  }
-  static cmListFileContext const empty;
-  return empty;
+  assert(this->TopEntry);
+  return this->TopEntry->Context;
 }
 
 void cmListFileBacktrace::PrintTitle(std::ostream& out) const
 {
-  if (!this->Cur) {
+  // The title exists only if we have a call on top of the bottom.
+  if (!this->TopEntry || this->TopEntry->IsBottom()) {
     return;
   }
-  cmOutputConverter converter(this->Bottom);
-  cmListFileContext lfc = *this->Cur;
-  if (!this->Bottom.GetState()->GetIsInTryCompile()) {
+  cmListFileContext lfc = this->TopEntry->Context;
+  cmStateSnapshot bottom = this->GetBottom();
+  cmOutputConverter converter(bottom);
+  if (!bottom.GetState()->GetIsInTryCompile()) {
     lfc.FilePath = converter.ConvertToRelativePath(
-      this->Bottom.GetState()->GetSourceDirectory(), lfc.FilePath);
+      bottom.GetState()->GetSourceDirectory(), lfc.FilePath);
   }
   out << (lfc.Line ? " at " : " in ") << lfc;
 }
 
 void cmListFileBacktrace::PrintCallStack(std::ostream& out) const
 {
-  if (!this->Cur || !this->Cur->Up) {
+  // The call stack exists only if we have at least two calls on top
+  // of the bottom.
+  if (!this->TopEntry || this->TopEntry->IsBottom() ||
+      this->TopEntry->Parent->IsBottom()) {
     return;
   }
 
   bool first = true;
-  cmOutputConverter converter(this->Bottom);
-  for (Entry* i = this->Cur->Up; i; i = i->Up) {
-    if (i->Name.empty()) {
+  cmStateSnapshot bottom = this->GetBottom();
+  cmOutputConverter converter(bottom);
+  for (Entry const* cur = this->TopEntry->Parent.get(); !cur->IsBottom();
+       cur = cur->Parent.get()) {
+    if (cur->Context.Name.empty()) {
       // Skip this whole-file scope.  When we get here we already will
       // have printed a more-specific context within the file.
       continue;
@@ -436,10 +419,10 @@ void cmListFileBacktrace::PrintCallStack(std::ostream& out) const
       first = false;
       out << "Call Stack (most recent call first):\n";
     }
-    cmListFileContext lfc = *i;
-    if (!this->Bottom.GetState()->GetIsInTryCompile()) {
+    cmListFileContext lfc = cur->Context;
+    if (!bottom.GetState()->GetIsInTryCompile()) {
       lfc.FilePath = converter.ConvertToRelativePath(
-        this->Bottom.GetState()->GetSourceDirectory(), lfc.FilePath);
+        bottom.GetState()->GetSourceDirectory(), lfc.FilePath);
     }
     out << "  " << lfc << "\n";
   }
@@ -448,16 +431,19 @@ void cmListFileBacktrace::PrintCallStack(std::ostream& out) const
 size_t cmListFileBacktrace::Depth() const
 {
   size_t depth = 0;
-  if (this->Cur == nullptr) {
-    return 0;
-  }
-
-  for (Entry* i = this->Cur->Up; i; i = i->Up) {
-    depth++;
+  if (Entry const* cur = this->TopEntry.get()) {
+    for (; !cur->IsBottom(); cur = cur->Parent.get()) {
+      ++depth;
+    }
   }
   return depth;
 }
 
+bool cmListFileBacktrace::Empty() const
+{
+  return !this->TopEntry || this->TopEntry->IsBottom();
+}
+
 std::ostream& operator<<(std::ostream& os, cmListFileContext const& lfc)
 {
   os << lfc.FilePath;

Source/cmListFileCache.h

@@ -6,6 +6,7 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <iosfwd>
+#include <memory> // IWYU pragma: keep
 #include <stddef.h>
 #include <string>
 #include <vector>
@@ -115,18 +116,20 @@ public:
   // Default-constructed backtrace may not be used until after
   // set via assignment from a backtrace constructed with a
   // valid snapshot.
-  cmListFileBacktrace();
+  cmListFileBacktrace() = default;
 
   // Construct an empty backtrace whose bottom sits in the directory
   // indicated by the given valid snapshot.
   cmListFileBacktrace(cmStateSnapshot const& snapshot);
 
-  // Backtraces may be copied and assigned as values.
-  cmListFileBacktrace(cmListFileBacktrace const& r);
-  cmListFileBacktrace& operator=(cmListFileBacktrace const& r);
-  ~cmListFileBacktrace();
+  // Backtraces may be copied, moved, and assigned as values.
+  cmListFileBacktrace(cmListFileBacktrace const&) = default;
+  cmListFileBacktrace(cmListFileBacktrace&&) noexcept = default;
+  cmListFileBacktrace& operator=(cmListFileBacktrace const&) = default;
+  cmListFileBacktrace& operator=(cmListFileBacktrace&&) noexcept = default;
+  ~cmListFileBacktrace() = default;
 
-  cmStateSnapshot GetBottom() const { return this->Bottom; }
+  cmStateSnapshot GetBottom() const;
 
   // Get a backtrace with the given file scope added to the top.
   // May not be called until after construction with a valid snapshot.
@@ -153,14 +156,15 @@ public:
   // Get the number of 'frames' in this backtrace
   size_t Depth() const;
 
+  // Return true if this backtrace is empty.
+  bool Empty() const;
+
 private:
   struct Entry;
-
-  cmStateSnapshot Bottom;
-  Entry* Cur;
-  cmListFileBacktrace(cmStateSnapshot const& bottom, Entry* up,
+  std::shared_ptr<Entry const> TopEntry;
+  cmListFileBacktrace(std::shared_ptr<Entry const> parent,
                       cmListFileContext const& lfc);
-  cmListFileBacktrace(cmStateSnapshot const& bottom, Entry* cur);
+  cmListFileBacktrace(std::shared_ptr<Entry const> top);
 };
 
 struct cmListFile