Make the diagnostic-missing-prototypes put the suggested `static` in front of `const` if exists.

[oontvoo]

Summary

Consider: const int* get_foo() {return nullptr;}
The suggested fix should be static const int* get_foo(){}
and not const static int* get_foo(){}

[gribozavr2]

clang/lib/Sema/SemaDecl.cpp

14252

Could you add tests for the following cases? I'm not sure the implementation will work.

// Two spaces between 'const' and 'struct'.
const  struct MyStruct get_struct() ...

const /*comment*/ struct MyStruct get_struct() ...

#define MY_CONST const
MY_CONST struct MyStruct get_struct() ...

I think a better way would be to check the token that comes before the type name, and if that token is 'const', then adjust the source location -- otherwise insert the 'static' keyword where we used to insert it in the past.

[oontvoo]

• Less brittle way to find the loc of const.
• Add more tests

[oontvoo]

clang/lib/Sema/SemaDecl.cpp

14252

Done.

[gribozavr2]

clang/lib/Lex/Lexer.cpp

1302 ↗

(On Diff #269627)

I'm concerned about putting this API into Lexer, given this implementation. It is not a precise general-purpose implementation, it can have false negatives (for example, if the const token is formed through macros, or token pasting or whatnot), and it can have false positives (if the 'const' token is actually a macro that expands to something else).

I'd suggest to define this function as a local lambda right where it is used or inline it completely into findBeginLoc.

clang/test/Sema/warn-missing-prototypes.c

57

Why not return (void*)0;?

62

Did you mean to return a pointer to MyStruct? The top-level const is not meaningful.

63

Extra indentation? Please make it consistent with other tests.

71

cost => const

98

"Since we can't easily understand what MY_CONST means while preparing the diagnostic, the fix-it suggests to add 'static' in a non-idiomatic place."

[oontvoo]

clang/test/Sema/warn-missing-prototypes.c

62

No, I actually intended to test the top-level const. Though not being meaningful, it's still syntactically correct.
But now that you've mentioned it, I should add a test for MyStruct* too.

[oontvoo]

Address review comment

clang/lib/Sema/SemaDecl.cpp

if (ShouldWarnAboutMissingPrototype(FD, PossiblePrototype)) {
             Diag(PossiblePrototype->getLocation(),
                  diag::note_declaration_not_a_prototype)
                 << (FD->getNumParams() != 0)
                 << (FD->getNumParams() == 0
                         ? FixItHint::CreateInsertion(FTL.getRParenLoc(), "void")
                         : FixItHint{});
         }
       } else {
+        // Returns true if the token beginning at this Loc is `const`.
+        auto isLocAtConst = [&](SourceLocation Loc, const SourceManager &SM,
+                                const LangOptions &LangOpts) {
+          std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Loc);
+          if (LocInfo.first.isInvalid())
+            return false;
+
+          bool Invalid = false;
+          StringRef Buffer = SM.getBufferData(LocInfo.first, &Invalid);
+          if (Invalid)
+            return false;
+
+          if (LocInfo.second > Buffer.size())
+            return false;
+
+          const char *LexStart = Buffer.data() + LocInfo.second;
+          StringRef StartTok(LexStart, Buffer.size() - LocInfo.second);
+
+          return StartTok.consume_front("const") &&
+                 (StartTok.empty() || isWhitespace(StartTok[0]) ||
+                  StartTok.startswith("/*") || StartTok.startswith("//"));
+        };
+
+        auto findBeginLoc = [&]() {
+          // If the return type has `const` qualifier, we want to insert
+          // `static` before `const` (and not before the typename).
+          if ((FD->getReturnType()->isAnyPointerType() &&
+               FD->getReturnType()->getPointeeType().isConstQualified()) ||
+              FD->getReturnType().isConstQualified()) {
+            // But only do this if we can determine where the `const` is.
+
+            if (isLocAtConst(FD->getBeginLoc(), getSourceManager(),
+                             getLangOpts()))
+
+              return FD->getBeginLoc();
+          }
+          return FD->getTypeSpecStartLoc();
+        };
         Diag(FD->getTypeSpecStartLoc(), diag::note_static_for_internal_linkage)
             << /* function */ 1
             << (FD->getStorageClass() == SC_None
-                    ? FixItHint::CreateInsertion(FD->getTypeSpecStartLoc(),
-                                                 "static ")
+                    ? FixItHint::CreateInsertion(findBeginLoc(), "static ")
                     : FixItHint{});
       }
 
       // GNU warning -Wstrict-prototypes
       //   Warn if K&R function is defined without a previous declaration.
       //   This warning is issued only if the definition itself does not provide
       //   a prototype. Only K&R definitions do not provide a prototype.
       if (!FD->hasWrittenPrototype()) {

clang/test/Sema/warn-missing-prototypes.c

 int f2(int x) { return x; }
 
 // rdar://6759522
 int main(void) { return 0; }
 
 void not_a_prototype_test(); // expected-note{{this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:27-[[@LINE-1]]:27}:"void"
 void not_a_prototype_test() { } // expected-warning{{no previous prototype for function 'not_a_prototype_test'}}
+
+const int *get_const() { // expected-warning{{no previous prototype for function 'get_const'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"static "
+  return (void *)0;
+}
+
+struct MyStruct {};
+
+const struct MyStruct get_struct() { // expected-warning{{no previous prototype for function 'get_struct'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"static "
+  struct MyStruct ret;
+  return ret;
+}
+
+// Turn off clang-format for white-space dependent testing.
+// clang-format off
+// Two spaces between cost and struct
+const  struct MyStruct get_struct_2() {  // expected-warning{{no previous prototype for function 'get_struct_2'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"static "
+  struct MyStruct ret;
+  return ret;
+}
+
+// Two spaces bewteen const and struct
+const  struct MyStruct* get_struct_ptr() {  // expected-warning{{no previous prototype for function 'get_struct_ptr'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"static "
+  return (void*)0;
+}
+
+// Random comment before const.
+/*some randome comment*/const  struct MyStruct* get_struct_3() {  // expected-warning{{no previous prototype for function 'get_struct_3'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:"static "
+  return (void*)0;
+}
+
+// Random comment after const.
+const/*some randome comment*/ struct MyStruct* get_struct_4() {  // expected-warning{{no previous prototype for function 'get_struct_4'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"static "
+  return (void*)0;
+}
+// clang-format on
+
+#define MY_CONST const
+
+// Since we can't easily understand what MY_CONST means while preparing the
+// diagnostic, the fix-it suggests to add 'static' in a non-idiomatic place.
+MY_CONST struct MyStruct *get_struct_nyi() { // expected-warning{{no previous prototype for function 'get_struct_nyi'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"static "
+  return (void *)0;
+}