[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

A few general things: don't modify FileSpec, we have many of these objects and we can't increase their size without directly affecting memory usage. FileSpec objects represent one file on disk, not multiple. We should make a new class that contains a FileSpec and a regex, but not put it into FileSpec.



================
Comment at: include/lldb/Utility/FileSpec.h:96
 
+  explicit FileSpec(llvm::StringRef regex);
+
----------------
This constructor seem dangerous as it might be confused with a path.


================
Comment at: include/lldb/Utility/FileSpec.h:589
       m_syntax; ///< The syntax that this path uses (e.g. Windows / Posix)
+  RegularExpression m_regex;          ///< Regular expression in "regex:" prefix passed.
 };
----------------
We shouldn't add something to FileSpec here. Now every FileSpec in LLDB will carry around an extra RegularExpression object as part of the object? Please remove. We should make a new class instead of adding something to FileSpec. FileSpec represents a single file on disk, not N files on disk.


================
Comment at: source/Commands/CommandObjectBreakpoint.cpp:566
+      case 'z':
+        m_filenames.AppendIfUnique(FileSpec(option_arg));
+        break;
----------------
We should probably have m_file_regexes (arrary) or just m_file_regex (one).


================
Comment at: source/Commands/CommandObjectBreakpoint.cpp:570
+      case 'Z':
+        m_modules.AppendIfUnique(FileSpec(option_arg));
+        break;
----------------
We should probably have m_module_regexes (arrary) or just m_module_regex (one).


================
Comment at: source/Utility/FileSpec.cpp:183-188
+FileSpec::FileSpec(llvm::StringRef regex) {
+  m_syntax = ePathSyntaxHostNative;
+  m_filename.SetString(regex);
+  m_regex.Compile(regex);
+}
+
----------------
Revert. We can't add stuff to FileSpec because some piece of code wants to use it in a non-standard way. FileSpec represents one file.


================
Comment at: source/Utility/FileSpec.cpp:194-195
     : m_directory(rhs.m_directory), m_filename(rhs.m_filename),
-      m_is_resolved(rhs.m_is_resolved), m_syntax(rhs.m_syntax) {}
+      m_is_resolved(rhs.m_is_resolved), m_syntax(rhs.m_syntax),
+      m_regex(rhs.m_regex) {}
 
----------------
Revert. We can't add stuff to FileSpec because some piece of code wants to use it in a non-standard way. FileSpec represents one file.


================
Comment at: source/Utility/FileSpec.cpp:219
     m_syntax = rhs.m_syntax;
+    m_regex = rhs.m_regex;
   }
----------------
Revert


================
Comment at: source/Utility/FileSpec.cpp:312-314
+  if (m_regex.IsValid())
+    return m_regex.Execute(rhs.GetPath());
+
----------------
Revert



================
Comment at: source/Utility/FileSpec.cpp:425-428
+  if (a.m_regex.IsValid())
+    return a.m_regex.Execute(b.GetPath());
+
   static ConstString g_dot_string(".");
----------------
revert


https://reviews.llvm.org/D39436