<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 1, 2014 at 6:47 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: alp<br>
Date: Tue Jul 1 20:47:15 2014<br>
New Revision: 212154<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=212154&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=212154&view=rev</a><br>
Log:<br>
Introduce a FunctionDecl::getReturnTypeSourceRange() utility<br>
<br>
This source range is useful for all kinds of diagnostic QOI and refactoring<br>
work, so let's make it more discoverable.<br>
<br>
This commit also makes use of the new function to enhance various diagnostics<br>
relating to return types and resolves an old FIXME.<br>
<br>
Modified:<br>
cfe/trunk/include/clang/AST/Decl.h<br>
cfe/trunk/lib/AST/Decl.cpp<br>
cfe/trunk/lib/Sema/SemaDecl.cpp<br>
cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>
<br>
Modified: cfe/trunk/include/clang/AST/Decl.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=212154&r1=212153&r2=212154&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=212154&r1=212153&r2=212154&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/AST/Decl.h (original)<br>
+++ cfe/trunk/include/clang/AST/Decl.h Tue Jul 1 20:47:15 2014<br>
@@ -1896,6 +1896,8 @@ public:<br>
return getType()->getAs<FunctionType>()->getReturnType();<br>
}<br>
<br>
+ SourceRange getReturnTypeSourceRange() const;<br>
+<br>
/// \brief Determine the type of an expression that calls this function.<br>
QualType getCallResultType() const {<br>
return getType()->getAs<FunctionType>()->getCallResultType(getASTContext());<br>
<br>
Modified: cfe/trunk/lib/AST/Decl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=212154&r1=212153&r2=212154&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=212154&r1=212153&r2=212154&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/AST/Decl.cpp (original)<br>
+++ cfe/trunk/lib/AST/Decl.cpp Tue Jul 1 20:47:15 2014<br>
@@ -2687,6 +2687,23 @@ bool FunctionDecl::doesDeclarationForceE<br>
return FoundBody;<br>
}<br>
<br>
+SourceRange FunctionDecl::getReturnTypeSourceRange() const {<br>
+ const TypeSourceInfo *TSI = getTypeSourceInfo();<br>
+ if (!TSI)<br>
+ return SourceRange();<br>
+<br>
+ TypeLoc TL = TSI->getTypeLoc();<br>
+ FunctionTypeLoc FunctionTL = TL.getAs<FunctionTypeLoc>();<br></blockquote><div><br></div><div>You should walk through ParenTypeLocs here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (!FunctionTL)<br>
+ return SourceRange();<br>
+<br>
+ TypeLoc ResultTL = FunctionTL.getReturnLoc();<br>
+ if (ResultTL.getUnqualifiedLoc().getAs<BuiltinTypeLoc>())<br>
+ return ResultTL.getSourceRange();<br>
+<br>
+ return SourceRange();<br>
+}<br>
+<br>
/// \brief For an inline function definition in C, or for a gnu_inline function<br>
/// in C++, determine whether the definition will be externally visible.<br>
///<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=212154&r1=212153&r2=212154&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=212154&r1=212153&r2=212154&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Jul 1 20:47:15 2014<br>
@@ -2519,11 +2519,13 @@ bool Sema::MergeFunctionDecl(FunctionDec<br>
ResQT = Context.mergeObjCGCQualifiers(NewQType, OldQType);<br>
if (ResQT.isNull()) {<br>
if (New->isCXXClassMember() && New->isOutOfLine())<br>
- Diag(New->getLocation(),<br>
- diag::err_member_def_does_not_match_ret_type) << New;<br>
+ Diag(New->getLocation(), diag::err_member_def_does_not_match_ret_type)<br>
+ << New << New->getReturnTypeSourceRange();<br>
else<br>
- Diag(New->getLocation(), diag::err_ovl_diff_return_type);<br>
- Diag(OldLocation, PrevDiag) << Old << Old->getType();<br>
+ Diag(New->getLocation(), diag::err_ovl_diff_return_type)<br>
+ << New->getReturnTypeSourceRange();<br>
+ Diag(OldLocation, PrevDiag) << Old << Old->getType()<br>
+ << Old->getReturnTypeSourceRange();<br>
return true;<br>
}<br>
else<br>
@@ -7494,8 +7496,10 @@ Sema::ActOnFunctionDeclarator(Scope *S,<br>
<br>
// OpenCL v1.2, s6.9 -- Kernels can only have return type void.<br>
if (!NewFD->getReturnType()->isVoidType()) {<br>
- Diag(D.getIdentifierLoc(),<br>
- diag::err_expected_kernel_void_return_type);<br>
+ SourceRange RTRange = NewFD->getReturnTypeSourceRange();<br>
+ Diag(D.getIdentifierLoc(), diag::err_expected_kernel_void_return_type)<br>
+ << (RTRange.isValid() ? FixItHint::CreateReplacement(RTRange, "void")<br>
+ : FixItHint());<br></blockquote><div><br></div><div>This fixit is not correct; it will do horrible things to</div><div><br></div><div> void __kernel (*f())[3];</div><div><br></div><div>It seems pretty tricky to determine whether all the tokens in the range are part of the return type, but that's what you'd need here.</div>
<div><br></div><div>It's probably not too bad to have incorrect fixits in corner cases on notes (because such fixits don't get automatically applied), but on an error we shouldn't really be producing incorrect fixits.</div>
<div><br></div><div>(I see below that we were already getting this wrong in some cases, so this isn't really a significant regression, but it's still something we ought to fix.)</div><div><br></div><div>Suggestion: Add a flag to this function to say whether the caller wants only a contiguous sequence of tokens. If that flag is set, inspect the TypeLoc of the return type to see if there might be intervening tokens, and return SourceRange() if so -- so "vector<int> f()" is ok, but "const vector<int> f()" is not, because we can't easily tell if there's an 'inline' keyword between the 'const' and the 'vector'. We can later fix this to give a valid range in more cases.</div>
<div><br></div><div>Perhaps this contiguous-tokens functionality should live on TypeLoc? It seems like a reasonable thing for people to want.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
D.setInvalidType();<br>
}<br>
<br>
@@ -7835,23 +7839,6 @@ bool Sema::CheckFunctionDeclaration(Scop<br>
return Redeclaration;<br>
}<br>
<br>
-static SourceRange getResultSourceRange(const FunctionDecl *FD) {<br>
- const TypeSourceInfo *TSI = FD->getTypeSourceInfo();<br>
- if (!TSI)<br>
- return SourceRange();<br>
-<br>
- TypeLoc TL = TSI->getTypeLoc();<br>
- FunctionTypeLoc FunctionTL = TL.getAs<FunctionTypeLoc>();<br>
- if (!FunctionTL)<br>
- return SourceRange();<br>
-<br>
- TypeLoc ResultTL = FunctionTL.getReturnLoc();<br>
- if (ResultTL.getUnqualifiedLoc().getAs<BuiltinTypeLoc>())<br>
- return ResultTL.getSourceRange();<br>
-<br>
- return SourceRange();<br>
-}<br>
-<br>
void Sema::CheckMain(FunctionDecl* FD, const DeclSpec& DS) {<br>
// C++11 [basic.start.main]p3:<br>
// A program that [...] declares main to be inline, static or<br>
@@ -7904,19 +7891,17 @@ void Sema::CheckMain(FunctionDecl* FD, c<br>
} else if (getLangOpts().GNUMode && !getLangOpts().CPlusPlus) {<br>
Diag(FD->getTypeSpecStartLoc(), diag::ext_main_returns_nonint);<br>
<br>
- SourceRange ResultRange = getResultSourceRange(FD);<br>
- if (ResultRange.isValid())<br>
- Diag(ResultRange.getBegin(), diag::note_main_change_return_type)<br>
- << FixItHint::CreateReplacement(ResultRange, "int");<br>
+ SourceRange RTRange = FD->getReturnTypeSourceRange();<br>
+ if (RTRange.isValid())<br>
+ Diag(RTRange.getBegin(), diag::note_main_change_return_type)<br>
+ << FixItHint::CreateReplacement(RTRange, "int");<br>
<br>
// Otherwise, this is just a flat-out error.<br>
} else {<br>
- SourceRange ResultRange = getResultSourceRange(FD);<br>
- if (ResultRange.isValid())<br>
- Diag(FD->getTypeSpecStartLoc(), diag::err_main_returns_nonint)<br>
- << FixItHint::CreateReplacement(ResultRange, "int");<br>
- else<br>
- Diag(FD->getTypeSpecStartLoc(), diag::err_main_returns_nonint);<br>
+ SourceRange RTRange = FD->getReturnTypeSourceRange();<br>
+ Diag(FD->getTypeSpecStartLoc(), diag::err_main_returns_nonint)<br>
+ << (RTRange.isValid() ? FixItHint::CreateReplacement(RTRange, "int")<br>
+ : FixItHint());<br>
<br>
FD->setInvalidDecl(true);<br>
}<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=212154&r1=212153&r2=212154&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=212154&r1=212153&r2=212154&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Tue Jul 1 20:47:15 2014<br>
@@ -12304,8 +12304,10 @@ bool Sema::CheckOverridingFunctionReturn<br>
if (NewClassTy.isNull()) {<br>
Diag(New->getLocation(),<br>
diag::err_different_return_type_for_overriding_virtual_function)<br>
- << New->getDeclName() << NewTy << OldTy;<br>
- Diag(Old->getLocation(), diag::note_overridden_virtual_function);<br>
+ << New->getDeclName() << NewTy << OldTy<br>
+ << New->getReturnTypeSourceRange();<br>
+ Diag(Old->getLocation(), diag::note_overridden_virtual_function)<br>
+ << Old->getReturnTypeSourceRange();<br>
<br>
return true;<br>
}<br>
@@ -12325,25 +12327,27 @@ bool Sema::CheckOverridingFunctionReturn<br>
if (!Context.hasSameUnqualifiedType(NewClassTy, OldClassTy)) {<br>
// Check if the new class derives from the old class.<br>
if (!IsDerivedFrom(NewClassTy, OldClassTy)) {<br>
- Diag(New->getLocation(),<br>
- diag::err_covariant_return_not_derived)<br>
- << New->getDeclName() << NewTy << OldTy;<br>
- Diag(Old->getLocation(), diag::note_overridden_virtual_function);<br>
+ Diag(New->getLocation(), diag::err_covariant_return_not_derived)<br>
+ << New->getDeclName() << NewTy << OldTy<br>
+ << New->getReturnTypeSourceRange();<br>
+ Diag(Old->getLocation(), diag::note_overridden_virtual_function)<br>
+ << Old->getReturnTypeSourceRange();<br>
return true;<br>
}<br>
<br>
// Check if we the conversion from derived to base is valid.<br>
- if (CheckDerivedToBaseConversion(NewClassTy, OldClassTy,<br>
- diag::err_covariant_return_inaccessible_base,<br>
- diag::err_covariant_return_ambiguous_derived_to_base_conv,<br>
- // FIXME: Should this point to the return type?<br>
- New->getLocation(), SourceRange(), New->getDeclName(),<br>
- nullptr)) {<br>
+ if (CheckDerivedToBaseConversion(<br>
+ NewClassTy, OldClassTy,<br>
+ diag::err_covariant_return_inaccessible_base,<br>
+ diag::err_covariant_return_ambiguous_derived_to_base_conv,<br>
+ New->getLocation(), New->getReturnTypeSourceRange(),<br>
+ New->getDeclName(), nullptr)) {<br>
// FIXME: this note won't trigger for delayed access control<br>
// diagnostics, and it's impossible to get an undelayed error<br>
// here from access control during the original parse because<br>
// the ParsingDeclSpec/ParsingDeclarator are still in scope.<br>
- Diag(Old->getLocation(), diag::note_overridden_virtual_function);<br>
+ Diag(Old->getLocation(), diag::note_overridden_virtual_function)<br>
+ << Old->getReturnTypeSourceRange();<br>
return true;<br>
}<br>
}<br>
@@ -12352,8 +12356,10 @@ bool Sema::CheckOverridingFunctionReturn<br>
if (NewTy.getLocalCVRQualifiers() != OldTy.getLocalCVRQualifiers()) {<br>
Diag(New->getLocation(),<br>
diag::err_covariant_return_type_different_qualifications)<br>
- << New->getDeclName() << NewTy << OldTy;<br>
- Diag(Old->getLocation(), diag::note_overridden_virtual_function);<br>
+ << New->getDeclName() << NewTy << OldTy<br>
+ << New->getReturnTypeSourceRange();<br>
+ Diag(Old->getLocation(), diag::note_overridden_virtual_function)<br>
+ << Old->getReturnTypeSourceRange();<br>
return true;<br>
};<br>
<br>
@@ -12362,8 +12368,10 @@ bool Sema::CheckOverridingFunctionReturn<br>
if (NewClassTy.isMoreQualifiedThan(OldClassTy)) {<br>
Diag(New->getLocation(),<br>
diag::err_covariant_return_type_class_type_more_qualified)<br>
- << New->getDeclName() << NewTy << OldTy;<br>
- Diag(Old->getLocation(), diag::note_overridden_virtual_function);<br>
+ << New->getDeclName() << NewTy << OldTy<br>
+ << New->getReturnTypeSourceRange();<br>
+ Diag(Old->getLocation(), diag::note_overridden_virtual_function)<br>
+ << Old->getReturnTypeSourceRange();<br>
return true;<br>
};<br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>