[PATCH]rs6000: avoid peeking eof after __vector keyword

Jiufu Guo guojiufu@linux.ibm.com
Wed Mar 23 03:37:55 GMT 2022


Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Tue, Mar 22, 2022 at 01:50:39PM +0800, Jiufu Guo wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> > On Mon, Mar 21, 2022 at 02:14:08PM -0400, David Edelsohn wrote:
>> >> On Mon, Mar 21, 2022 at 5:13 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote:
>> >> > There is a rare corner case: where __vector is followed only with ";"
>> >> > and near the end of the file.
>> >
>> >> This is okay. Maybe a tweak to the comment, see below.
>> >
>> > This whole function could use some restructuring / rewriting to make
>> > clearer what it actually does.  See the function comment:
>> >
>> > /* Called to decide whether a conditional macro should be expanded.
>> >    Since we have exactly one such macro (i.e, 'vector'), we do not
>> >    need to examine the 'tok' parameter.  */
>> >
>> > ... followed by 17 uses of "tok".  Yes, some of those overwrite the
>> > function argument, but that doesn't make it any better!  :-P
>> >
>> > Some factoring would help, too, perhaps.
>> 
>> Thanks for your review!
>> 
>> I am also confused about it when I check this function for the first
>> time. In the function, 'tok' is used directly at the beginning, and
>> then it is overwritten by cpp_peek_token.
>> >From the history of this function, the first version of this function
>> contains this 'inconsistency' between comments and implementations. :-P
>> 
>> With check related code, it seems this function is used to predicate
>> if a conditional macro should be expanded by peeking two or more
>> tokens.
>
> Yes, and the function comment should say that, that's what it's for :-)
>
>> The context-sensitive macros are vector/bool/pixel.  Correponding
>> keywords __vector/__bool/__pixel are unconditional.
>> Based on those related codes, the behavior of function
>> rs6000_macro_to_expand would be checking if the 'vector' token is
>> followed by bool/__bool or pixel/__pixel.  To do this the 'tok' has to
>> be 'examined'.
>> 
>> >From this understanding, we may just update the comment.
>> While the patch does not cover this.
>
> Yes, and it doesn't have to, probably it's best not to change the code
> much in stage 4 anyway.  It is really hard to fix bugs in it, or to
> review the resulting patches, without documentation what it is supposed
> to do (especially if the code isn't clear, is inconsistent, and even
> contradicts the little documentation that there is).

Thanks for your comments!

Understand. :)  I would commit the patch and submit patch to update
the comments in stage 1.

BR,
Jiufu

>
> Oh well.
>
>
> Segher


More information about the Gcc-patches mailing list