[cfe-commits] [Patch] Suggest typo corrections for implicit function decls
Richard Smith
richard at metafoo.co.uk
Fri Dec 2 17:33:14 PST 2011
On Fri, December 2, 2011 23:56, David Blaikie wrote:
> 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?
Yes. See http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-May/014903.html
Also, remember that -Werror will convert warnings into errors, and we don't
remove the fixits in that case, so warnings must follow the same rules as
errors.
> 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
>>
>
More information about the cfe-commits
mailing list