This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PR-10034 Bad space handling in `set remote exec-file' command.


On Tue, Oct 4, 2011 at 9:20 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
> Abhijit> In the `set remote exec-file' command if we provide space at the end
> Abhijit> of the file-name, the space is not being chopped off and being
> Abhijit> considered as part of file-name. This behavior is inconsistent across
> Abhijit> similar set commands like `set logging file' etc. My patch will fix
> Abhijit> that problem. Please review this patch and put your comments.
>
> I can't tell if you re-posted the same patch or if it has changes.
> Please:
>
> * If you are sending a ping, just send a ping, as a followup to the
> ?patch being pinged, so that it threads properly in a threading mail
> ?reader.
>
> * If you are sending a new patch, again send it as a followup, and also
> ?indicate what changed and why.
>
Okay, I will follow this in future.
>
> This patch doesn't have a test case, but should.
>
Okay, I will write a test-case.
> Abhijit> ? ? ? ?case var_string_noescape:
> Abhijit> - ? ? ? ?if (arg == NULL)
> Abhijit> - ? ? ? ? ?arg = "";
> Abhijit> - ? ? ? ?if (*(char **) c->var != NULL)
> Abhijit> - ? ? ? ? ?xfree (*(char **) c->var);
> Abhijit> - ? ? ? ?*(char **) c->var = xstrdup (arg);
> Abhijit> - ? ? ? ?break;
>
> Why are you changing this case?
>
The idea was that the same code block is common between two adjacent
cases and hence can be combined together.
> Abhijit> ? ? ? ?case var_optional_filename:
> Abhijit> ? ? ? ? ?if (arg == NULL)
> Abhijit> ? ? ? ? ? ?arg = "";
> Abhijit> + ? ? ? ?else
> Abhijit> + ? ? ? ? ?{
> Abhijit> + ? ? ? ? ? ?/* Clear trailing whitespace. ?*/
> Abhijit> + ? ? ? ? ? ?char *ptr = arg + strlen (arg) - 1;
> Abhijit> +
> Abhijit> + ? ? ? ? ? ?while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
> Abhijit> + ? ? ? ? ? ? ?ptr--;
> Abhijit> + ? ? ? ? ? ?*(ptr + 1) = '\0';
>
> Why not use remove_trailing_whitespace? ?You mentioned it in an earlier
> note, it seems best to just use it from the start.
>
Okay, I will do this change. I was just waiting for people's comment on this.
>
> What happens if the resulting string is empty?
> At least for var_filename this should give an error.
>
Okay.
> Abhijit> @@ -419,7 +423,7 @@ cmd_show_list (struct cmd_list_element *
> Abhijit> ? ?for (; list != NULL; list = list->next)
> Abhijit> ? ? ?{
> Abhijit> ? ? ? ?/* If we find a prefix, run its list, prefixing our output by its
> Abhijit> - ? ? ? ? prefix (with "show " skipped). ?*/
> Abhijit> + ? ? ? prefix (with "show " skipped). ?*/
> Abhijit> ? ? ? ?if (list->prefixlist && !list->abbrev_flag)
> Abhijit> ? ? ? ?{
> Abhijit> ? ? ? ? ?struct cleanup *optionlist_chain
>
> This hunk looks like a gratuitous change.
>
This is a 8 space to tab conversion that we followed in rest of the
code. If suggested I may revert back this change.
>
> Tom
>

Regards,
Abhijit Halder


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]