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

[reed kotler]

I have proposed a patch to clang in 
http://llvm.org/bugs/show_bug.cgi?id=15538

Do you want me to submit this patch?

For now I will make the test case for my patch and remove the offending 
comment.


On 03/18/2013 05:39 PM, David Blaikie wrote:
> On Mon, Mar 18, 2013 at 5:28 PM, reed kotler <rkotler at mips.com> wrote:
>> 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
>>
>> With my fix, everything works. Reverting my change will cause failures at Mips in our test-suite runs.
> My point would be not that this patch should be reverted, but that the
> original patch that added the buggy code be reverted until the bug/fix
> is properly understood.
>
>> That was just my opinion that I should not be called under these conditions.
> & that seems to indicate that you don't have a clear understanding of
> the code that's been written/committed - that doesn't seem like code
> we would want committed, does it?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130318/b86bbe9c/attachment.html>