Page MenuHomePhabricator

[SemaCXX] Param diagnostic matches overload logic
ClosedPublic

Authored by modocache on Jan 21 2019, 12:30 PM.

Details

Summary

Given the following test program:

class C {
public:
  int A(int a, int& b);
};

int C::A(const int a, int b) {
  return a * b;
}

Clang would produce an error message that correctly diagnosed the
redeclaration of C::A to not match the original declaration (the
parameters to the two declarations do not match -- the original takes an
int & as its 2nd parameter, but the redeclaration takes an int). However,
it also produced a note diagnostic that inaccurately pointed to the
first parameter, claiming that const int in the redeclaration did not
match the unqualified int in the original. The diagnostic is
misleading because it has nothing to do with why the program does not
compile.

The logic for checking for a function overload, in
Sema::FunctionParamTypesAreEqual, discards cv-qualifiers before
checking whether the types are equal. Do the same when producing the
overload diagnostic.

Diff Detail

Repository
rC Clang

Event Timeline

modocache created this revision.Jan 21 2019, 12:30 PM
cpplearner added inline comments.
lib/Sema/SemaDecl.cpp
5090

I see this is what Sema::FunctionParamTypesAreEqual does, but maybe both places should use hasSameUnqualifiedType?

modocache updated this revision to Diff 184284.Jan 30 2019, 6:35 AM

Thanks, @cpplearner! You're absolutely right. I got confused because I didn't realize that the method FunctionProtoType::getUnqualifiedType was being used from within Sema::FunctionParamTypesAreEqual, not QualType::getUnqualifiedType. I modified my patch to use ASTContext::hasSameUnqualifiedType instead.

modocache marked an inline comment as done.Jan 31 2019, 6:42 PM

Friendly ping! Is there anything I can do to improve this patch? I think it's a clear improvement of the diagnostic, with a test case to boot!

Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 6:42 PM
rsmith accepted this revision.Jan 31 2019, 6:57 PM

Thanks, LGTM.

This revision is now accepted and ready to land.Jan 31 2019, 6:57 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the reviews!