Re: Fwd: Delivery Status Notification (Failure)

[Doug Evans <xdje42 at gmail dot com>]

On Sun, Nov 23, 2014 at 11:18 AM, Doug Evans <xdje42@gmail.com> wrote:
> [+ keiths, since this is linespec related, and I know
> how much he loves linespecs!    :-)]
>
> On Thu, Nov 20, 2014 at 6:57 AM, David Taylor
> <dtaylor@usendtaylorx2l.lss.emc.com> wrote:
>> Doug Evans <xdje42@gmail.com> wrote:
>>> The "," in "-at LOCATION," seems a bit random, relative to other commands.
>>>
>>> Maybe it is the best way to go.
>>> If so, I'd like to see the reasons why it exists documented in the code.
>>>
>>> Can we just remove the , and require -- when necessary?
>>
>> It marks the end of the location and the start of the macro.  It is
>> patterned after
>>
>>     maint agent [-at location,] EXPRESSION
>> and
>>     maint agent-eval [-at location,] EXPRESSION
>>
>> I did it the same way for consistency.
>
> Ah.  Can't fault that. :-)
>
> Still, I'm more ok with u/i visible hacks in maint commands
> than with normal commands.
> Bubbling this up into non-maint commands means needing
> to now worry about consistency with all the other commands.
>
>> It seems unnecessary, but the location parsing code stops at comma but
>> not at space.  Presumably that is so that file names can contain spaces.
>> But, that is just a guess on my part.  I personally dislike file names
>> containing spaces.
>
> Such names need to be quoted.
> OTOH c++ functions with parameters, for example,
> don't (usually) need to be quoted.
>
> linespec.c has this:
> static const char * const linespec_keywords[] = { "if", "thread", "task" };
>
> This may not be the best solution, and it might involve a few more
> changes to linespec.c (or elsewhere) but that's one alternative:
> static const char * const linespec_keywords[] = { "if", "thread",
> "task", "--" };
>
> It works, but "--" wasn't intended for this purpose (mark the
> end of the linespec), so it's just a not-well-thought-out-idea.
>
> I tried to think of a situation where using a comma here would
> lead to u/i warts later, but couldn't.
> E.g., if the comma gets used in other contexts
> then will users start complaining that "b foo, thread 1" doesn't work?
> If that day comes we *could* allow the comma in that context.
> But it would be odd that a comma is *required* in some
> contexts and not in others.
>
> In the end, I'm ok with the patch, but
> I think the docs (both offline and online) need to highlight the comma
> as being a requirement for "-at" since it's not intuitive.
> Plus I think we need to document in some linespec-related
> place that "," is now required to be recognized as ending a linespec.
> I don't know if that exists today.
> [FAOD, a "," within a linespec is still ok,
> e.g., "info macro -at foo(int, int), BAR"]
> Ok with those changes.

Re: Plus I think we need to document in some linespec-related
place that "," is now required to be recognized as ending a linespec.

FAOD, I was thinking of, say, adding a comment to the
function-comment for decode_line_full in linespec.h.

E.g.,

--- linespec.h= 2014-11-23 10:30:23.566784217 -0800
+++ linespec.h  2014-11-23 11:55:35.531659067 -0800
@@ -133,7 +133,11 @@ extern struct symtabs_and_lines
    entry describing all the matching locations.  If FILTER is
    non-NULL, then only locations whose canonical name is equal (in the
    strcmp sense) to FILTER will be returned; all others will be
-   filtered out.  */
+   filtered out.
+
+   Note for new callers: If you need a way to mark the end of the linespec
+   in *ARGPTR, fyi the "info macro -at LOCATION, MACRO" command uses a comma.
+   A comma within the linespec is still ok (e.g., "foo (int, int)").  */

 extern void decode_line_full (char **argptr, int flags,
                              struct symtab *default_symtab, int default_line,

or some such.
The point being to make the comma terminator
being part of the spec of what's a valid linespec.


---
IWBN to "fix" the linespec parsing machinery to recognize
the end of the line spec so that the comma wasn't required.
I don't want to make that a prerequisite for this patch,
but down the road if that happens then the comma
could just become optional.