[libcxxabi] r238263 - Make sure !empty() before calling String::front().

[David Majnemer]

On Tue, May 26, 2015 at 7:22 PM, Chaoren Lin <chaorenl at google.com> wrote:

> Hi David,
>
> Thanks for your concern.
>
> The commit was intentional. I was hoping this small commit would fall
> under this part:
>
> smaller changes can be reviewed after commit
>
>
> of the LLVM Developer Policy
> <http://llvm.org/docs/DeveloperPolicy.html#code-reviews>
>

I'm afraid it doesn't because of the following proviso:

Code review can be an iterative process, which continues until the patch is
> ready to be committed. Specifically, once a patch is sent out for review,
> it needs an explicit “looks good” before it is submitted. Do not assume
> silent approval, or request active objections to the patch with a deadline.




> We inlined a copy of this file in LLDB for Windows, and I ran into a
> problem where any mangled names with pointers or references in their
> argument list would cause a crash. Turns out that on Linux, String::front()
> consistently returns '\0' when empty, even though it should be undefined
> behavior.
>

> It should be covered by existing tests if you enable asserts for libcxx
> and try to demangle "_Z1FPiRiOiMS0_FiiE".
>

I don't see an existing test which tries to demangle '_Z1FPiRiOiMS0_FiiE'.
I think you should add it to test/test_demangle.pass.cpp


>
> Thanks,
> Chaoren
>
> On Tue, May 26, 2015 at 4:43 PM, David Majnemer <david.majnemer at gmail.com>
> wrote:
>
>> I didn't see anyone LGTM the phabricator review and no test has been
>> added for this change.  Perhaps you accidentally committed this?
>>
>> On Tue, May 26, 2015 at 4:14 PM, Chaoren Lin <chaorenl at google.com> wrote:
>>
>>> Author: chaoren
>>> Date: Tue May 26 18:14:26 2015
>>> New Revision: 238263
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=238263&view=rev
>>> Log:
>>> Make sure !empty() before calling String::front().
>>>
>>> Reviewers: howard.hinnant
>>>
>>> Subscribers: cfe-commits
>>>
>>> Differential Revision: http://reviews.llvm.org/D9954
>>>
>>> Modified:
>>>     libcxxabi/trunk/src/cxa_demangle.cpp
>>>
>>> Modified: libcxxabi/trunk/src/cxa_demangle.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/libcxxabi/trunk/src/cxa_demangle.cpp?rev=238263&r1=238262&r2=238263&view=diff
>>>
>>> ==============================================================================
>>> --- libcxxabi/trunk/src/cxa_demangle.cpp (original)
>>> +++ libcxxabi/trunk/src/cxa_demangle.cpp Tue May 26 18:14:26 2015
>>> @@ -1671,7 +1671,7 @@ parse_pointer_to_member_type(const char*
>>>                  auto func = std::move(db.names.back());
>>>                  db.names.pop_back();
>>>                  auto class_type = std::move(db.names.back());
>>> -                if (func.second.front() == '(')
>>> +                if (!func.second.empty() && func.second.front() == '(')
>>>                  {
>>>                      db.names.back().first = std::move(func.first) + "("
>>> + class_type.move_full() + "::*";
>>>                      db.names.back().second = ")" +
>>> std::move(func.second);
>>> @@ -2018,7 +2018,8 @@ parse_type(const char* first, const char
>>>                                      db.names[k].first += " (";
>>>                                      db.names[k].second.insert(0, ")");
>>>                                  }
>>> -                                else if (db.names[k].second.front() ==
>>> '(')
>>> +                                else if (!db.names[k].second.empty() &&
>>> +                                          db.names[k].second.front() ==
>>> '(')
>>>                                  {
>>>                                      db.names[k].first += "(";
>>>                                      db.names[k].second.insert(0, ")");
>>> @@ -2045,7 +2046,8 @@ parse_type(const char* first, const char
>>>                                      db.names[k].first += " (";
>>>                                      db.names[k].second.insert(0, ")");
>>>                                  }
>>> -                                else if (db.names[k].second.front() ==
>>> '(')
>>> +                                else if (!db.names[k].second.empty() &&
>>> +                                          db.names[k].second.front() ==
>>> '(')
>>>                                  {
>>>                                      db.names[k].first += "(";
>>>                                      db.names[k].second.insert(0, ")");
>>> @@ -2079,7 +2081,8 @@ parse_type(const char* first, const char
>>>                                      db.names[k].first += " (";
>>>                                      db.names[k].second.insert(0, ")");
>>>                                  }
>>> -                                else if (db.names[k].second.front() ==
>>> '(')
>>> +                                else if (!db.names[k].second.empty() &&
>>> +                                          db.names[k].second.front() ==
>>> '(')
>>>                                  {
>>>                                      db.names[k].first += "(";
>>>                                      db.names[k].second.insert(0, ")");
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150526/f8c74dfa/attachment.html>