inclyc added inline comments.
================
Comment at: clang/test/Parser/while-loop-outside-function.c:8
+
+void some_fn();
+
----------------
inclyc wrote:
> mizvekov wrote:
> > inclyc wrote:
> > > mizvekov wrote:
> > > > Can you add a few more test cases showing how error recovery is
> > > > performing here?
> > > >
> > > > Have we parsed this function declaration at all, or were we skipping
> > > > until the next ';'?
> > > >
> > > > What happens if there are (multiple) statements in the loop's block?
> > > >
> > > > How do we handle a `do ... while()` loop instead?
> > > >
> > > > Does it make a difference if the loop contains a block or a (possibly
> > > > empty) single statement?
> > > For this occasion,
> > >
> > > ```
> > > // RUN: %clang_cc1 -fsyntax-only -verify %s
> > >
> > > while(true) {}; // expected-error {{while loop outside of function}}
> > >
> > > // without semicolon
> > > while(true) {} // expected-error {{while loop outside of function}}
> > >
> > >
> > > while(true) {
> > > // some statements
> > > int some_var = 3;
> > > some_var += 2;
> > > }
> > >
> > > while(true)
> > > {
> > > // some statements
> > > int some_var = 3;
> > > some_var += 2;
> > > }
> > >
> > > do {
> > > int some_var = 1;
> > > some_var += 3;
> > > }
> > > while(true);
> > >
> > > void someFunction() {
> > > while(true) {};
> > > }
> > >
> > > class SomeClass {
> > > public:
> > > while(true) {}
> > > void some_fn() {
> > > while(true) {}
> > > }
> > > };
> > >
> > > ```
> > >
> > > ```
> > > ./clang/test/Parser/while-loop-outside-function.cpp:3:1: error: while
> > > loop outside of function
> > > while(true) {}; // expected-error {{while loop outside of function}}
> > > ^
> > > ./clang/test/Parser/while-loop-outside-function.cpp:6:1: error: while
> > > loop outside of function
> > > while(true) {} // expected-error {{while loop outside of function}}
> > > ^
> > > ./clang/test/Parser/while-loop-outside-function.cpp:9:1: error: while
> > > loop outside of function
> > > while(true) {
> > > ^
> > > ./clang/test/Parser/while-loop-outside-function.cpp:15:1: error: while
> > > loop outside of function
> > > while(true)
> > > ^
> > > ./clang/test/Parser/while-loop-outside-function.cpp:22:1: error: expected
> > > unqualified-id
> > > do {
> > > ^
> > > ./clang/test/Parser/while-loop-outside-function.cpp:26:1: error: while
> > > loop outside of function
> > > while(true);
> > > ^
> > > ./clang/test/Parser/while-loop-outside-function.cpp:34:5: error: expected
> > > member name or ';' after declaration specifiers
> > > while(true) {}
> > > ^
> > > 7 errors generated.
> > > ```
> > >
> > > The parser generates the newly added error for every "top-level"
> > > declarator. For `do ... while(...)` loops it generates two errors. I
> > > think ideally only one error should be reported, only generating an error
> > > at the latter while token, without generating a "noise" at the preceding
> > > `do`. And, this patch doesn't seem to produce good results where classes,
> > > structs, etc. are also not part of the function body.
> > >
> > > At first, when I solved this issue, I thought he only needed to report
> > > the appearance of the top-level while loop, without considering do-while,
> > > in a class, or in a structure. So the current code is only in Parser,
> > > this error is reported when the `while` token is encountered parsing
> > > declarators.
> > I see, thanks for the explanations.
> >
> > I think this patch is a very simple change that brings some improvement,
> > and apparently it does not regress anything or cause any unfortunate error
> > recovery.
> >
> > You don't really need to improve these other cases in the same patch, I am
> > just checking that this was all considered :-)
> >
> > But please do add these test cases to the patch!
> Do you think we need to change something more to accurately report `while` /
> `do-while` / `for` loops, and only report errors outside of a reasonable
> control flow?
Right now this patch is far from perfect, ideally it should report various
types of loops (as mentioned above) and also in a non-control flow context,
such as class members, typedefs, etc. (I don't know how to do this, sorry)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129573/new/
https://reviews.llvm.org/D129573
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
