Segher Boessenkool <seg...@kernel.crashing.org> writes: > Hi! > > On Tue, Mar 22, 2022 at 01:50:39PM +0800, Jiufu Guo wrote: >> Segher Boessenkool <seg...@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 <guoji...@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
