Quantcast

Small fix of column number in parser

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Small fix of column number in parser

Daniel Kraft
Hi all,

attached is a hg changeset that fixes wrong column numbers.  If the
attached test.m is executed without the patch, one gets:

error: bar
error: called from:
error:   /tmp/test.m at line 2, column -1

With the patch, the column number is correctly reported as 1.

Note that this is somewhat the "minimal" version of the fix; since
skip_white_space is only used once (in gobble_white_space) and also the
stdio_stream_reader class is only used there, it may be nice to somewhat
unify the code.  Currently, line-numbers are tracked in
stdio_stream_reader and column-numbers in skip_white_space.

Do you prefer this simple patch or should I work on some "clean-up" of
this situation, too -- like making skip_white_space a member of
stdio_stream_reader and unifying column- and line-number tracking in one
place.

Yours,
Daniel

PS: Since this is my first attempt at both working with Mercurial and
submitting a patch for octave, please let me know if there's something I
should do different (and in general, comments welcome).  In particular,
is there some tag I should add to messages sent with patches?  E.g., on
the GCC lists, we have "[patch]" in the subjects of messages containing
diff's.

--
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri

test.m (18 bytes) Download Attachment
column.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

Jordi Gutiérrez Hermoso-2
On 5 April 2011 04:17, Daniel Kraft <[hidden email]> wrote:

> attached is a hg changeset that fixes wrong column numbers.  If the
> attached test.m is executed without the patch, one gets:
>
> error: bar
> error: called from:
> error:   /tmp/test.m at line 2, column -1
>
> With the patch, the column number is correctly reported as 1.

I tested your patch and it worked, so I pushed it to the default
branch:

    http://hg.savannah.gnu.org/hgweb/octave/rev/fd367312095a

> Do you prefer this simple patch or should I work on some "clean-up"
> of this situation, too -- like making skip_white_space a member of
> stdio_stream_reader and unifying column- and line-number tracking in
> one place.

I think for a first patch, a minimal solution is fine. Leave the
refactoring for later.

> PS: Since this is my first attempt at both working with Mercurial
> and submitting a patch for octave, please let me know if there's
> something I should do different (and in general, comments welcome).

You used mq; please submit finished patches (commits) next time, and
use a meaningful commit message. You also used our old-style ChangeLog
commit format, which we recently changed. Put the same information
that you put in the ChangeLog in the commit message. Here are the
guidelines for doing so:

    http://octave.1599824.n4.nabble.com/policy-for-pushing-changesets-tp3443512p3445355.html

>  In particular, is there some tag I should add to messages sent with
> patches?  E.g., on the GCC lists, we have "[patch]" in the subjects
> of messages containing diff's.

Yes, please use our patch tracker. Easier to have them all in one
place instead of scattered in our respected inboxen:

    https://savannah.gnu.org/patch/?group=octave

Thanks,
- Jordi G. H.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

John W. Eaton
Administrator
On 28-Apr-2011, Jordi Gutiérrez Hermoso wrote:

| On 5 April 2011 04:17, Daniel Kraft <[hidden email]> wrote:
|
| > attached is a hg changeset that fixes wrong column numbers.  If the
| > attached test.m is executed without the patch, one gets:
| >
| > error: bar
| > error: called from:
| > error:   /tmp/test.m at line 2, column -1
| >
| > With the patch, the column number is correctly reported as 1.
|
| I tested your patch and it worked, so I pushed it to the default
| branch:
|
|     http://hg.savannah.gnu.org/hgweb/octave/rev/fd367312095a

I don't understand why you removed the line that decremented the
column count when the character is put back on the input stream.
What problem does that fix?  It looks to me like removing that line
would introduce a bug.

| >  In particular, is there some tag I should add to messages sent with
| > patches?  E.g., on the GCC lists, we have "[patch]" in the subjects
| > of messages containing diff's.
|
| Yes, please use our patch tracker. Easier to have them all in one
| place instead of scattered in our respected inboxen:
|
|     https://savannah.gnu.org/patch/?group=octave

If the patch fixes a bug, then please attach the patch to a bug report
in the bug tracker.  I'd prefer to just use the patch tracker for new
functions or features, not for bug fixes.

Thanks,

jwe

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

Daniel Kraft
In reply to this post by Jordi Gutiérrez Hermoso-2
Hi Jordi,

On 04/28/11 07:54, Jordi Gutiérrez Hermoso wrote:

> On 5 April 2011 04:17, Daniel Kraft<[hidden email]>  wrote:
>
>> attached is a hg changeset that fixes wrong column numbers.  If the
>> attached test.m is executed without the patch, one gets:
>>
>> error: bar
>> error: called from:
>> error:   /tmp/test.m at line 2, column -1
>>
>> With the patch, the column number is correctly reported as 1.
>
> I tested your patch and it worked, so I pushed it to the default
> branch:
>
>      http://hg.savannah.gnu.org/hgweb/octave/rev/fd367312095a

thanks!  Regarding testing a patch before commit (or submitting it), is
there some "standard required way" -- i.e., running "make check" without
errors or something else?

>> Do you prefer this simple patch or should I work on some "clean-up"
>> of this situation, too -- like making skip_white_space a member of
>> stdio_stream_reader and unifying column- and line-number tracking in
>> one place.
>
> I think for a first patch, a minimal solution is fine. Leave the
> refactoring for later.

Ok.  So should I now work on a refactoring?  While I believe that the
code in question could become simpler, I also think that it is actually
only run in very rare circumstances (namely for the white-space at the
very beginning of a file); so it probably won't be that helpful, after all.

>> PS: Since this is my first attempt at both working with Mercurial
>> and submitting a patch for octave, please let me know if there's
>> something I should do different (and in general, comments welcome).
>
> You used mq; please submit finished patches (commits) next time, and
> use a meaningful commit message. You also used our old-style ChangeLog
> commit format, which we recently changed. Put the same information
> that you put in the ChangeLog in the commit message. Here are the
> guidelines for doing so:
>
>      http://octave.1599824.n4.nabble.com/policy-for-pushing-changesets-tp3443512p3445355.html

Ok about the commit message and ChangeLog change.  How can I convert my
mq queue to a finished patch?  I followed the workflow at
http://www.gnu.org/software/octave/doc/interpreter/How-to-Contribute.html#How-to-Contribute,
AFAIK.  (And never used hg before.)

Is it enough to just do a hg commit on the active queue before hg
export?  And if I do this, can I later get back to the patch, revise it
and submit a changed version in case something comes up in code review?

>>   In particular, is there some tag I should add to messages sent with
>> patches?  E.g., on the GCC lists, we have "[patch]" in the subjects
>> of messages containing diff's.
>
> Yes, please use our patch tracker. Easier to have them all in one
> place instead of scattered in our respected inboxen:
>
>      https://savannah.gnu.org/patch/?group=octave

Ah, I didn't know about that -- so far I'm used to submitting patches
for review on the mailing lists.  But in the future I'll happily use the
patch tracker, seems like a useful thing :)

Thanks a lot,
Daniel

--
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

Daniel Kraft
In reply to this post by John W. Eaton
On 04/28/11 08:13, John W. Eaton wrote:

> On 28-Apr-2011, Jordi Gutiérrez Hermoso wrote:
>
> | On 5 April 2011 04:17, Daniel Kraft<[hidden email]>  wrote:
> |
> |>  attached is a hg changeset that fixes wrong column numbers.  If the
> |>  attached test.m is executed without the patch, one gets:
> |>
> |>  error: bar
> |>  error: called from:
> |>  error:   /tmp/test.m at line 2, column -1
> |>
> |>  With the patch, the column number is correctly reported as 1.
> |
> | I tested your patch and it worked, so I pushed it to the default
> | branch:
> |
> |     http://hg.savannah.gnu.org/hgweb/octave/rev/fd367312095a
>
> I don't understand why you removed the line that decremented the
> column count when the character is put back on the input stream.
> What problem does that fix?  It looks to me like removing that line
> would introduce a bug.

As I understand it, this should be right -- it's more or less the other
way round, if we skip a white-space character, we increment the counter.
  If we don't have white-space, we put it back on the stream, return and
do nothing at all to the counter.  Decrementing it was precisely the
reason for reporting "column -1" above.

> |>    In particular, is there some tag I should add to messages sent with
> |>  patches?  E.g., on the GCC lists, we have "[patch]" in the subjects
> |>  of messages containing diff's.
> |
> | Yes, please use our patch tracker. Easier to have them all in one
> | place instead of scattered in our respected inboxen:
> |
> |     https://savannah.gnu.org/patch/?group=octave
>
> If the patch fixes a bug, then please attach the patch to a bug report
> in the bug tracker.  I'd prefer to just use the patch tracker for new
> functions or features, not for bug fixes.

There was no bug report for this problem -- but of course I can always
create one to attach a patch which fixes a bug.  In that case, should I
also post to the mailing list that a new patch is available, or can this
be seen right from the bug-list?

Yours,
Daniel

--
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

John W. Eaton
Administrator
On 28-Apr-2011, Daniel Kraft wrote:

| As I understand it, this should be right -- it's more or less the other
| way round, if we skip a white-space character, we increment the counter.
|   If we don't have white-space, we put it back on the stream, return and
| do nothing at all to the counter.  Decrementing it was precisely the
| reason for reporting "column -1" above.

OK, I understand now.  I think your patch is fine.

| There was no bug report for this problem -- but of course I can always
| create one to attach a patch which fixes a bug.  In that case, should I
| also post to the mailing list that a new patch is available, or can this
| be seen right from the bug-list?

There's usually no need to post a message to the list if you report a
bug in the tracker.  People who care to follow the bug reports are
already subscribed to the notification list for the tracker.

jwe
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

Daniel Kraft
On 04/28/11 08:56, John W. Eaton wrote:
> On 28-Apr-2011, Daniel Kraft wrote:
> | There was no bug report for this problem -- but of course I can always
> | create one to attach a patch which fixes a bug.  In that case, should I
> | also post to the mailing list that a new patch is available, or can this
> | be seen right from the bug-list?
>
> There's usually no need to post a message to the list if you report a
> bug in the tracker.  People who care to follow the bug reports are
> already subscribed to the notification list for the tracker.

Ah, ok then!

Cheers,
Daniel

--
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

Jordi Gutiérrez Hermoso-2
In reply to this post by Daniel Kraft
2011/4/28 Daniel Kraft <[hidden email]>:

> Regarding testing a patch before commit (or submitting it), is there
> some "standard required way" -- i.e., running "make check" without
> errors or something else?

Yes, "make check" should be part of your testing, make sure you didn't
break something inadvertently.

>>> Do you prefer this simple patch or should I work on some
>>> "clean-up" of this situation, too -- like making skip_white_space
>>> a member of stdio_stream_reader and unifying column- and
>>> line-number tracking in one place.
>>
>> I think for a first patch, a minimal solution is fine. Leave the
>> refactoring for later.
>
> Ok. So should I now work on a refactoring?

No, I don't think this problem is worth it. Do you disagree? Find
bigger fish to fry.

> Ok about the commit message and ChangeLog change.  How can I convert
> my mq queue to a finished patch?

The "hg qfinish" will turn your patch into a commit. Before you do
that, you should edit your commit message with "hg qref -e" to do so
in an editor or "hg qref -m 'foo (bar): baz'" if it's a short
one-liner. You can read the hg docstrings for the foo command by
typing "hg help foo".

> Is it enough to just do a hg commit on the active queue before hg
> export? And if I do this, can I later get back to the patch, revise
> it and submit a changed version in case something comes up in code
> review?

If you need to move your commit back to the hg queue, you can do so
with "hg qimport -r $patch_rev", then follow whatever mq workflow
works for you.

Mercurial is a great DVCS; I really like it. I think you should get
yourself comfortable with it. There's a free book online that is quite
comprehensive. You probably would also enjoy a graphical frontend to
it like TortoiseHg which is handy for visualising your repo's DAG. If
you need help with it, you can consult the mailing lists or its
channel in Freenode; both have been very welcoming to me.

HTH,
- Jordi G. H.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small fix of column number in parser

Daniel Kraft
On 04/28/11 16:13, Jordi Gutiérrez Hermoso wrote:
> 2011/4/28 Daniel Kraft<[hidden email]>:
>
>> Regarding testing a patch before commit (or submitting it), is there
>> some "standard required way" -- i.e., running "make check" without
>> errors or something else?
>
> Yes, "make check" should be part of your testing, make sure you didn't
> break something inadvertently.

Ok, I'll remember this for all future patches ;)

>>>> Do you prefer this simple patch or should I work on some
>>>> "clean-up" of this situation, too -- like making skip_white_space
>>>> a member of stdio_stream_reader and unifying column- and
>>>> line-number tracking in one place.
>>>
>>> I think for a first patch, a minimal solution is fine. Leave the
>>> refactoring for later.
>>
>> Ok. So should I now work on a refactoring?
>
> No, I don't think this problem is worth it. Do you disagree? Find
> bigger fish to fry.

I totally agree here.

And thanks for the hg tips!

Yours,
Daniel

--
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri
Loading...