Re: [PATCH]rs6000: avoid peeking eof after __vector keyword

[Segher Boessenkool <segher@kernel.crashing.org>]

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).

Oh well.


Segher