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

[Don Hinton via Phabricator via lldb-commits]

hintonda added inline comments.


================
Comment at: include/lldb/Utility/FileSpec.h:65-69
+  enum PathSyntax : unsigned char {
     ePathSyntaxPosix,
     ePathSyntaxWindows,
     ePathSyntaxHostNative
   };
----------------
zturner wrote:
> This is actually a very nice change, as it reduces the size of `FileSpec` by a couple of bytes.  I think you can submit this change as a one-liner by itself, independent of this patch.
I suppose it depends on you compiler/OS, but this by it self doesn't change the size of FileSpec at all -- just changes the padding from 3 to 6.  It's still the size of 3 pointers due to alignment -- at least that's my understanding.

However, if you did have a way to encode this stuff into the two existing pointers, it might help -- you still need at least 4 bits if I'm not mistaken.


================
Comment at: include/lldb/Utility/FileSpec.h:557
   mutable bool m_is_resolved = false; ///< True if this path has been resolved.
-  PathSyntax
-      m_syntax; ///< The syntax that this path uses (e.g. Windows / Posix)
+  bool m_is_regex = false;            ///< Filename is a regular expression.
 };
----------------
zturner wrote:
> I thought in previous comments we had decided that this wasn't really the right direction, and that `FileSpec` should represent one file.  //If// we want this functionality in LLDB (and again, I'm not convinced), it should be done in such a way that the `FileSpec` class remains unmodified.  We should not have to touch this class for any of this.
I actually spent quite a bit of time trying to do it the other way, i.e., not touching FileSpec, but the diff got so big, I decided it was probably a mistake.  I still have it in a local branch, but it's not ready to commit.



https://reviews.llvm.org/D39436