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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/test/Parser/while-loop-outside-function.c:10-19
+// empty statement without semicolon
+while(1) // expected-error {{while loop outside of a function}}
+{
+
+}
+
+// empty statement without semicolon, inline l_paren
----------------
I think you don't really need to test these two cases separately. They differ by just white space.

The one thing that I did suggest is that, for at least one of your test cases, put the while keyword alone by itself on a line,
so that you can test that the diagnostic points to the location of the keyword.


================
Comment at: clang/test/Parser/while-loop-outside-function.c:8
+
+void some_fn();
+
----------------
inclyc wrote:
> 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)
Don't worry about it! I think this patch is pretty good bang for the buck, for the cost of basically two lines of code you get much better diagnostic for the bulk of the interesting cases, without regressing anything.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129573/new/

https://reviews.llvm.org/D129573