[PATCH] D129573: [Clang] add a diagnostic note 'while loop outside functions' at global scope

[YingChi Long via Phabricator via cfe-commits]

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