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

hintonda added a comment. In https://reviews.llvm.org/D39436#912810, @clayborg wrote: > I was unhappy when we went over two pointers for a FileSpec when m_syntax was > added due to the extra size. Anything we can do to make this smaller would be > great, so the type on the enum would work, but

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

hintonda added inline comments. Comment at: include/lldb/Utility/FileSpec.h:65-69 + enum PathSyntax : unsigned char { ePathSyntaxPosix, ePathSyntaxWindows, ePathSyntaxHostNative }; zturner wrote: > hintonda wrote: > > zturner wrote: > > > This

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

zturner added inline comments. Comment at: include/lldb/Utility/FileSpec.h:65-69 + enum PathSyntax : unsigned char { ePathSyntaxPosix, ePathSyntaxWindows, ePathSyntaxHostNative }; hintonda wrote: > zturner wrote: > > This is actually a very

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

hintonda abandoned this revision. hintonda added a comment. Okay, got the message. Sorry for the noise. https://reviews.llvm.org/D39436 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

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

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

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. IIUC, Zachary, Greg and I have all said we don't think adding regular expressions patterns to FileSpec is a good idea. I'm not sure why you are pursuing this avenue. Of course,

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

labath added a comment. In case there isn't enough people disagreeing, I'd like to say that I also think this does not belong in FileSpec. If for nothing else, then for the FileSpec::Equal implementation: the semantics of the IsEqual method are complicated enough to follow even without having

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

zturner added inline comments. Comment at: include/lldb/Utility/FileSpec.h:65-69 + enum PathSyntax : unsigned char { ePathSyntaxPosix, ePathSyntaxWindows, ePathSyntaxHostNative }; This is actually a very nice change, as it reduces the size of

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

hintonda updated this revision to Diff 121214. hintonda added a comment. Addressed @clayborg's comments. - Added FileSpec ctor that takes an explicit RegularExpression object when using FileSpec as a regex pattern. - Removed the RegularExpression member variable, added a flag, and create the

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

hintonda added a comment. In https://reviews.llvm.org/D39436#912910, @zturner wrote: > >> I haven't had time to really look into this, but it seems that maintaining >> two independent strings, one for directory and one for basename, is just for >> convenience. We could easily keep it in

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

clayborg added a comment. And yes, the memory savings are quite large as well when sharing directories. https://reviews.llvm.org/D39436 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

clayborg added a comment. The main reason for two strings is for searching efficiency. Most people don't set breakpoints using full paths, they give the basename: (lldb) b main.c:12 When setting breakpoints is it very easy to search for matches by basename since this is what users usually

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

hintonda added a comment. In https://reviews.llvm.org/D39436#912829, @clayborg wrote: > In https://reviews.llvm.org/D39436#912828, @zturner wrote: > > > In https://reviews.llvm.org/D39436#912810, @clayborg wrote: > > > > > I was unhappy when we went over two pointers for a FileSpec when m_syntax

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

> On Nov 1, 2017, at 9:54 AM, Greg Clayton via Phabricator > wrote: > > clayborg added a comment. > > In https://reviews.llvm.org/D39436#912828, @zturner wrote: > >> In https://reviews.llvm.org/D39436#912810, @clayborg wrote: >> >>> I was unhappy when we went over

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

clayborg added a comment. In https://reviews.llvm.org/D39436#912828, @zturner wrote: > In https://reviews.llvm.org/D39436#912810, @clayborg wrote: > > > I was unhappy when we went over two pointers for a FileSpec when m_syntax > > was added due to the extra size. Anything we can do to make this

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

zturner added a comment. In https://reviews.llvm.org/D39436#912810, @clayborg wrote: > I was unhappy when we went over two pointers for a FileSpec when m_syntax was > added due to the extra size. Anything we can do to make this smaller would be > great, so the type on the enum would work, but

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

clayborg added a comment. I was unhappy when we went over two pointers for a FileSpec when m_syntax was added due to the extra size. Anything we can do to make this smaller would be great, so the type on the enum would work, but as you say the alignment will nullify that. The two ConstString

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

hintonda added a comment. Actually, it wouldn't matter for this one due to alignment. https://reviews.llvm.org/D39436 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

hintonda added a comment. In https://reviews.llvm.org/D39436#911875, @clayborg wrote: > 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

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

On Tue, Oct 31, 2017 at 10:43 AM Jim Ingham wrote: > > > > On Oct 31, 2017, at 9:05 AM, Zachary Turner wrote: > > > > This is a case where I think a new API *would* be warranted. But it > would not be an API specifically for the -m option. It would

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

Not directly, though you can get the ID from the breakpoint you've made, and then do: debugger.HandleCommand("breakpoint modify -m 0 %d"%(ID)) Jim > On Oct 31, 2017, at 8:32 AM, Don Hinton wrote: > > Btw, is there a way to pass the '-m' option via the SB API? I'd like

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

> On Oct 31, 2017, at 9:05 AM, Zachary Turner wrote: > > This is a case where I think a new API *would* be warranted. But it would > not be an API specifically for the -m option. It would instead be an API > that introduces an `SBBreakpointOptions` class, and then add a

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

This is a case where I think a new API *would* be warranted. But it would not be an API specifically for the -m option. It would instead be an API that introduces an `SBBreakpointOptions` class, and then add a method called `BreakpointCreateWithOptions(options)`. Note that the general policy of

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

Btw, is there a way to pass the '-m' option via the SB API? I'd like to exclude matches in comments when using BreakpointCreateBySourceRegex. On Tue, Oct 31, 2017 at 8:26 AM, Don Hinton wrote: > On Tue, Oct 31, 2017 at 8:22 AM, Zachary Turner > wrote: >

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

On Tue, Oct 31, 2017 at 8:22 AM, Zachary Turner wrote: > > > On Tue, Oct 31, 2017 at 8:12 AM Don Hinton wrote: > >> There have been a few suggestions that I could just use a script to solve >> this "problem" -- poor startup performance of clangdiag. >> >>

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

On Tue, Oct 31, 2017 at 8:12 AM Don Hinton wrote: > There have been a few suggestions that I could just use a script to solve > this "problem" -- poor startup performance of clangdiag. > > However, this patch was not submitted to solve a particular problem. It > was

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

There have been a few suggestions that I could just use a script to solve this "problem" -- poor startup performance of clangdiag. However, this patch was not submitted to solve a particular problem. It was submitted in response to Jim's suggestion: On Mon, Oct 23, 2017 at 6:25 PM, Jim Ingham

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

I'll add tests if it looks like it'll be accepted, but based on the initial response, that doesn't seem likely. However, it was a good exercise and addressed the issues raised. thanks again for all the feedback... don On Mon, Oct 30, 2017 at 9:44 PM, Zachary Turner wrote:

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

Asking again, but why can’t this be done in th the script for clangdiag? For example, there’s no tests for any of this in this patch. And it seems likely that it will be rarely used anyway. So I’m still not convinced the option-pollution and increased long term code maintenance burden of this

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

hintonda updated this revision to Diff 120933. hintonda added a comment. - Remove prefix and add options. https://reviews.llvm.org/D39436 Files: include/lldb/Utility/FileSpec.h source/Commands/CommandObjectBreakpoint.cpp source/Utility/FileSpec.cpp Index: source/Utility/FileSpec.cpp

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

On Mon, Oct 30, 2017 at 5:47 PM Jim Ingham via Phabricator < revi...@reviews.llvm.org> wrote: > jingham added a comment. > > In https://reviews.llvm.org/D39436#911305, @zturner wrote: > > > In https://reviews.llvm.org/D39436#911302, @hintonda wrote: > > > > > In

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

> On Oct 30, 2017, at 5:35 PM, Zachary Turner wrote: > > > On Mon, Oct 30, 2017 at 5:32 PM Jim Ingham via Phabricator > > wrote: > jingham added a comment. > > In https://reviews.llvm.org/D39436#911304 >

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

jingham added a comment. In https://reviews.llvm.org/D39436#911305, @zturner wrote: > In https://reviews.llvm.org/D39436#911302, @hintonda wrote: > > > In https://reviews.llvm.org/D39436#911264, @jingham wrote: > > > > > Zachary's suggestion is better than adding regex patterns to FileSpec, > >

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

On Mon, Oct 30, 2017 at 5:32 PM Jim Ingham via Phabricator < revi...@reviews.llvm.org> wrote: > jingham added a comment. > > In https://reviews.llvm.org/D39436#911304, @hintonda wrote: > > > In https://reviews.llvm.org/D39436#911274, @jingham wrote: > > > > > BTW, to Z's comment: you can't really

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

jingham added a comment. In https://reviews.llvm.org/D39436#911304, @hintonda wrote: > In https://reviews.llvm.org/D39436#911274, @jingham wrote: > > > BTW, to Z's comment: you can't really resolve the regex pattern when you > > make the breakpoint. What if another library gets loaded later

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

hintonda added a comment. In https://reviews.llvm.org/D39436#911296, @jingham wrote: > In https://reviews.llvm.org/D39436#911293, @hintonda wrote: > > > In https://reviews.llvm.org/D39436#911260, @jingham wrote: > > > > > I don't like this change. > > > > > > First off, the whole point of having

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

hintonda added a comment. In https://reviews.llvm.org/D39436#911274, @jingham wrote: > BTW, to Z's comment: you can't really resolve the regex pattern when you make > the breakpoint. What if another library gets loaded later on, which has > source files that match the source file pattern the

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

hintonda added a comment. In https://reviews.llvm.org/D39436#911264, @jingham wrote: > Zachary's suggestion is better than adding regex patterns to FileSpec, but I > still don't like the idea of encoding option types in the option values. Are you talking about fnmatch? Is that portable? If

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

jingham added a comment. I do think "all inlines of line 7 of my_cool_templates.h in some_file.cpp" is potentially useful. So adding a "source-pattern" and using it as a limiter for a search in much the same way --shlib is used seems generally useful to me. So it seems worthwhile adding this

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

jingham added a comment. In https://reviews.llvm.org/D39436#911293, @hintonda wrote: > In https://reviews.llvm.org/D39436#911260, @jingham wrote: > > > I don't like this change. > > > > First off, the whole point of having options in the commands is so that we > > don't have to have magic

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

hintonda added a comment. In https://reviews.llvm.org/D39436#911260, @jingham wrote: > I don't like this change. > > First off, the whole point of having options in the commands is so that we > don't have to have magic encodings in the values. > > We also don't have FileSpec's that resolve to

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

hintonda added a comment. In https://reviews.llvm.org/D39436#911263, @zturner wrote: > In https://reviews.llvm.org/D39436#911254, @hintonda wrote: > > > In https://reviews.llvm.org/D39436#911237, @zturner wrote: > > > > > Hmm, weird. Maybe it already is? Even though I didn't see it on the > >

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

zturner added a comment. In https://reviews.llvm.org/D39436#911264, @jingham wrote: > Zachary's suggestion is better than adding regex patterns to FileSpec, but I > still don't like the idea of encoding option types in the option values. > > Moreover, this doesn't really apply to the main use

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

jingham added a comment. Zachary's suggestion is better than adding regex patterns to FileSpec, but I still don't like the idea of encoding option types in the option values. Moreover, this doesn't really apply to the main use of -f - file & line breakpoints. The only plausible use for a

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

jingham added a comment. BTW, to Z's comment: you can't really resolve the regex pattern when you make the breakpoint. What if another library gets loaded later on, which has source files that match the source file pattern the user entered, and have source code that matches the -p pattern.

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

zturner added a comment. In https://reviews.llvm.org/D39436#911254, @hintonda wrote: > In https://reviews.llvm.org/D39436#911237, @zturner wrote: > > > Hmm, weird. Maybe it already is? Even though I didn't see it on the > > original email. Anyway, ignore my last suggestion. > > > > On to

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

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. I don't like this change. First off, the whole point of having options in the commands is so that we don't have to have magic encodings in the values. We also don't have

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

hintonda added a comment. In https://reviews.llvm.org/D39436#911233, @zturner wrote: > Would you mind deleting and re-creating this revision with lldb-commits added > as a subscriber? I don't think it's sufficient to just "add" it as a > subscriber after the fact, I think it has to be done as

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

zturner added a comment. Hmm, weird. Maybe it already is? Even though I didn't see it on the original email. Anyway, ignore my last suggestion. On to actual comments: I'm not really crazy about the `regex:` prefix. Seems ugly and non-intuitive. Couldn't we just treat every filename as an

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

zturner added a comment. Would you mind deleting and re-creating this revision with lldb-commits added as a subscriber? I don't think it's sufficient to just "add" it as a subscriber after the fact, I think it has to be done as part of the initial creation, or for some reason it won't show up