On Tue, Nov 16, 2021 at 09:28:21PM -0500, David Malcolm wrote:
> On Tue, 2021-11-16 at 19:37 -0500, Marek Polacek wrote:
> > Sorry for a dumb question, but is this what you have in mind?
> >
> > /* LRE
> > PDF */
> > /* FSI
> > PDI */
> > and check that we warn for these?
>
> I mean something like the following multiline comments in which lines
> within them at the start, middle and end have unpaired constructs
> within a given line:
>
>
> /* RLI
> *
> */
>
> /*
> * RLI
> */
>
> /*
> *
> * RLI */
>
> and that we should warn for each case at the line containing the
> unpaired control character.
>
> (the above lines don't have the actual chars, just "RLI")
>
> Mostly this is just me trying to think about it from a black-box
> testing perspective, or in case we ever touch this code in the future
> (perhaps it's obviously correct by inspection of the implementation
> now, but let's have regression tests for these cases).
>
> Sorry to add more work, but here's an idea for another test case:
> multiple comments on one line:
>
> /* RLI */ /* PDF */
>
> where the closure of a comment should trigger closing a "context", so
> we should complain about the above.
No problem, I've added these.
> >
> > > > > > @@ -1505,13 +1855,17 @@ lex_identifier (cpp_reader *pfile,
> > > > > > const uchar *base, bool starts_ucn,
> > > > > > {
> > > > > > /* Slower version for identifiers containing UCNs
> > > > > > or extended chars (including $). */
> > > > > > - do {
> > > > > > - while (ISIDNUM (*pfile->buffer->cur))
> > > > > > - {
> > > > > > - NORMALIZE_STATE_UPDATE_IDNUM (nst, *pfile->buffer-
> > > > > > >cur);
> > > > > > - pfile->buffer->cur++;
> > > > > > - }
> > > > > > - } while (forms_identifier_p (pfile, false, nst));
> > > > > > + do
> > > > > > + {
> > > > > > + while (ISIDNUM (*pfile->buffer->cur))
> > > > > > + {
> > > > > > + NORMALIZE_STATE_UPDATE_IDNUM (nst, *pfile-
> > > > > > >buffer->cur);
> > > > > > + pfile->buffer->cur++;
> > > > > > + }
> > > > > > + }
> > > > > > + while (forms_identifier_p (pfile, false, nst));
> > > > >
> > > > > Is the above purely a whitespace change?
> > > >
> > > > Yes.
> > >
> > > If I'm reading things correctly, these lines in the existing code
> > > were
> > > correctly indented, so is there a purpose to this change? If not,
> > > please can you remove this change from the patch (to minimize the
> > > change to the history).
> >
> > I dropped that change then. Sometimes it's hard to resist fixing
> > formatting. ;)
>
> Thanks. But I don't think the existing formatting in the code *was*
> broken; I thought the patch was taking correct formatting and breaking
> it (hence my objection to a whitespace change). If I misread this,
> sorry.
I think it was, we're supposed to format do-while as
do
{
}
while (...);
but it's obviously not a big deal.
> Hopefully the above makes sense and is constructive; let me know when
> you push your patch so that I can work on my followup.
Pushed now. Thanks!
Marek
