r177329 - This code works around what appears to be a bug in another part of clang.

[reed kotler]

On 03/18/2013 05:18 PM, David Blaikie wrote:
> On Mon, Mar 18, 2013 at 3:18 PM, Reed Kotler <rkotler at mips.com> wrote:
>> Author: rkotler
>> Date: Mon Mar 18 17:18:00 2013
>> New Revision: 177329
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
>> Log:
>> This code works around what appears to be a bug in another part of clang.
>> I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
>> This code is safer anyway because "cast" assumes you really know that
>> it's okay to make the cast. In this case isa should not be false and
>> dyn_cast should not return null as far as I understand. But everything
>> else is valid so I did not want to revert my previous patch for attributes
>> mips16/nomips16 or use an llvm_unreachable here which would make a number
>> of our tests fail for mips.
>>
>>
>> Modified:
>>      cfe/trunk/lib/CodeGen/TargetInfo.cpp
>>
>> Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
>> @@ -4323,7 +4323,8 @@ public:
>>                              CodeGen::CodeGenModule &CGM) const {
>>       const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
>>       if (!FD) return;
>> -    llvm::Function *Fn = cast<llvm::Function>(GV);
>> +    llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
>> +    if (!Fn) return; // should not happen
> Do you have a test case to commit with this?
>
> Also - I tend to agree with Rafael: if you don't know why you need to
> make a change, then it's probably not OK to make this fix. Sounds like
> you don't understand what's going on here & are papering over it in a
> way that seems to work but without any basis to believe that your
> change is right/correct other than "it works". We don't really develop
> code this way.
>
> If your reviewers told you to write it some way that doesn't work,
> discuss it with them.
>
> If other targets have the same bug, that's not necessarily sufficient
> justification to add another instance of the same bug once we know it
> is a bug.
>
> We aren't punishing you for being honest, we're asking you to not
> commit buggy/insufficiently understood/justified code.
>
> Checking in an uncertain fix while you investigate the problem isn't
> the usual practice on the project - we tend to revert a patch until we
> can it's correct. (granted, if a bug has been in the codebase long
> enough we don't trawl back through the commits & rip out the
> patch/functionality that added it - so, yes, the longer a patch is in
> the more likely that when we find a bug we'll just let the tree
> continue to be broken until we commit a fix for the bug - in this case
> it sounds like your contribution is still fresh enough to be a fix
> (with a known correct fix) or revert kind of situation)
>
>>       if (FD->hasAttr<Mips16Attr>()) {
>>         Fn->addFnAttr("mips16");
>>       }
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
The fix may be to just eliminate the comment

  // should not happen

That was just my opinion that I should not be called under these conditions.