[cfe-commits] [Patch] Suggest typo corrections for implicit function decls

[David Blaikie]

On Fri, Dec 2, 2011 at 3:09 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Hi Hans,
>
> On Sat, November 26, 2011 19:29, Hans Wennborg wrote:
>> I noticed that C programmers don't get to enjoy all of Clang's typo
>> corrections: a mistyped function call becomes an implicit function
>> declaration (with a warning). The attached patch adds typo correction to the
>> already existing warnings.
>>
>> Please take a look and let me know if it's okay for me to land this.
>
> Sorry for the delay. Some comments below.
>
> diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
> index cd2f071..4d45e32 100644
> --- a/lib/Sema/SemaDecl.cpp
> +++ b/lib/Sema/SemaDecl.cpp
> @@ -7262,13 +7262,49 @@ NamedDecl
> *Sema::ImplicitlyDefineFunction(SourceLocation Loc,
>     return Pos->second;
>   }
>
> +  // See if we can find a typo correction.
> +  TypoCorrection Corrected;
> +  FunctionDecl* Func = NULL;
>
> Please put the space to the right of the *, not to the left. Also, the null
> pointer constant is written as 0 more frequently than as NULL in Sema.
>
> +  std::string CorrectedStr;
> +  std::string CorrectedQuotedStr;
> +  if (S && (Corrected = CorrectTypo(DeclarationNameInfo(&II, Loc),
> +                                    LookupOrdinaryName, S, 0))) {
> +    // Since this is an implicit function declaration, we are only
> +    // interested in a potential typo for a function name.
> +    if ((Func = dyn_cast_or_null<FunctionDecl>(
> +            Corrected.getCorrectionDecl()))) {
> +      CorrectedStr = Corrected.getAsString(getLangOptions());
> +      CorrectedQuotedStr = Corrected.getQuoted(getLangOptions());
> +    }
> +  }
> +
>   // Extension in C99.  Legal in C90, but warn about it.
> -  if (II.getName().startswith("__builtin_"))
> -    Diag(Loc, diag::warn_builtin_unknown) << &II;
> -  else if (getLangOptions().C99)
> -    Diag(Loc, diag::ext_implicit_function_decl) << &II;
> -  else
> -    Diag(Loc, diag::warn_implicit_function_decl) << &II;
> +  // If we found a typo correction, then suggest that.
> +  if (Func) {
> +    unsigned diagnostic_kind;
> +    if (II.getName().startswith("__builtin_"))
> +      diagnostic_kind = diag::warn_builtin_unknown_suggest;
> +    else if (getLangOptions().C99)
> +      diagnostic_kind = diag::ext_implicit_function_decl_suggest;
> +    else
> +      diagnostic_kind = diag::warn_implicit_function_decl_suggest;
> +
> +    Diag(Loc, diagnostic_kind) << &II << CorrectedQuotedStr
> +      << FixItHint::CreateReplacement(Loc, CorrectedStr);
>
> Please move this fix-it to a separate note (at least for the implicit function
> declaration case -- see below). When a fix-it is issued on a warning or an
> error, we must recover as if the fixed code had been parsed (and for a
> warning, this essentially means the fix-it must not change the semantics).

I don't believe this can be/is a requirement for warnings, is it?
Otherwise most fixits on warnings wouldn't be applicable (unless they
were just fixits to suppress the warning but not change the
semantics). I believe that "recover exactly as typed" requirement only
exists for fixits on errors.

> +
> +    if (Func->getLocation().isValid())
> +      Diag(Func->getLocation(), diag::note_previous_decl) << CorrectedQuotedStr;
> +  } else {
> +    unsigned diagnostic_kind;
> +    if (II.getName().startswith("__builtin_"))
> +      diagnostic_kind = diag::warn_builtin_unknown;
> +    else if (getLangOptions().C99)
> +      diagnostic_kind = diag::ext_implicit_function_decl;
> +    else
> +      diagnostic_kind = diag::warn_implicit_function_decl;
> +
> +    Diag(Loc, diagnostic_kind) << &II;
> +  }
>
>   // Set a Declarator for the implicit definition: int foo();
>   const char *Dummy;
> diff --git a/test/Misc/warning-flags.c b/test/Misc/warning-flags.c
> index 5fc48cd..87a982f 100644
> --- a/test/Misc/warning-flags.c
> +++ b/test/Misc/warning-flags.c
> @@ -17,7 +17,7 @@ This test serves two purposes:
>
>  The list of warnings below should NEVER grow.  It should gradually shrink to 0.
>
> -CHECK: Warnings without flags (274):
> +CHECK: Warnings without flags (275):
>
> New warnings without flags should not be added (not even if you're just
> splitting a warning in two -- please add a flag; we all have taxes to pay).
> However, it strikes me that a Warning, DefaultError message with no -W flag,
> like this one, is a strange construct indeed, and it should probably simply be
> changed into an Error. With that done, you can keep the fix-it on the error
> for the __builtin_ case, if you also recover as if the user had typed the
> correct builtin name.
>
>  CHECK-NEXT:   ext_anon_param_requires_type_specifier
>  CHECK-NEXT:   ext_anonymous_struct_union_qualified
>  CHECK-NEXT:   ext_array_init_copy
> @@ -129,6 +129,7 @@ CHECK-NEXT:   warn_bitfield_width_exceeds_type_size
>  CHECK-NEXT:   warn_bool_switch_condition
>  CHECK-NEXT:   warn_braces_around_scalar_init
>  CHECK-NEXT:   warn_builtin_unknown
> +CHECK-NEXT:   warn_builtin_unknown_suggest
>  CHECK-NEXT:   warn_c_kext
>  CHECK-NEXT:   warn_call_to_pure_virtual_member_function_from_ctor_dtor
>  CHECK-NEXT:   warn_call_wrong_number_of_arguments
> diff --git a/test/Sema/builtins.c b/test/Sema/builtins.c
> index 64efefc..92a49bd 100644
> --- a/test/Sema/builtins.c
> +++ b/test/Sema/builtins.c
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -triple=i686-apple-darwin9
> +// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -fno-spell-checking
> -triple=i686-apple-darwin9
>
> Rather than turning off spell-checking, it'd be better to update the test to
> expect the new output (and add more cases here if we've lost some coverage).
>
>  // This test needs to set the target because it uses __builtin_ia32_vec_ext_v4si
>
>  int test1(float a, int b) {
> diff --git a/test/Sema/implicit-decl.c b/test/Sema/implicit-decl.c
> index f455977..f4b9016 100644
> --- a/test/Sema/implicit-decl.c
> +++ b/test/Sema/implicit-decl.c
> @@ -3,6 +3,8 @@
>  typedef int int32_t;
>  typedef unsigned char Boolean;
>
> +extern int printf(__const char *__restrict __format, ...); //
> expected-note{{'printf' declared here}}
> +
>  void func() {
>    int32_t *vector[16];
>    const char compDesc[16 + 1];
> @@ -10,6 +12,10 @@ void func() {
>    if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { //
> expected-note {{previous implicit declaration is here}} \
>          expected-warning {{implicit declaration of function
> '_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
>    }
> +
> +   printg("Hello, World!\n"); // expected-warning{{implicit declaration of
> function
> 'printg' is invalid in C99; did you mean 'printf'?}}
> +
> +  __builtin_is_les(1, 3); // expected-error{{use of unknown builtin
> '__builtin_is_les'; did you mean '__builtin_is_less'?}}
>  }
>  Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t
> **vector, int32_t count) { // expected-error{{conflicting types for
> '_CFCalendarDecomposeAbsoluteTimeV'}}
>  return 0;
>
> Could you add a test for the C89 case, please?
>
> Thanks,
> Richard
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits