Re: r321099 - [driver][darwin] Take the OS version specified in "-target" as the target
> On Dec 21, 2017, at 16:39, Alex L wrote:
>
>
>
>> On 21 December 2017 at 12:34, James Y Knight wrote:
>> I totally agree with moving towards eliminating the -m-version-min
>> flags, it's much better to put it in the target, and will clean up a lot of
>> cruft in the driver, eventually.
>>
>> Now -- we (or anyone else who runs into this) can simply start specifying
>> the version in both locations ("-target x86_64-apple-ios10
>> -mios-simulator-version-min=10"), so as to work with both new and old clang,
>> and be closer to the ultimate goal of having only -target. That's an overall
>> nicer workaround to suggest than switching to -darwin. But, yea, there's no
>> need for *temporary* patch to fix things just for us.
>>
>> However, I do not understand why you're against committing the patch you
>> mention as option #2 -- that seems like it'd be best for all users, by
>> preserving compatibility with existing command-lines. So, I'd still like
>> that change to be committed, permanently, not temporarily. I'm sure we can't
>> be the only ones running clang like "-target x86_64-apple-ios
>> -mios-simulator-version-min=10", and it seems unfortunate and unnecessary to
>> break that, even if it can be worked around.
>>
>> In the future, I'd hope the -m-version-min arguments can be deprecated
>> more and more -- warning whenever you use them to modify the platform or
>> version at all, rather just on specification conflict; then, warn anytime
>> you use them at all; then, remove them. But in the meantime, it seems
>> strictly better to preserve compatibility, don't you think?
>
> + Duncan
>
> Thanks, I think your argument is convincing. I think I will commit the change
> that you're proposing in the near future if there are no further objections.
SGTM.
>
>
>>
>>
>>
>> On Dec 21, 2017 2:11 PM, "Alex L" wrote:
>> Thanks for raising your concerns.
>>
>> We decided to avoid -m-version-min flag in favor of -target to simplify
>> the driver logic and to encourage the adoption of -target. Now after r321145
>> we only warn about -m-version-min flag when the OS version specified in
>> it is different to the OS version specified in target, or when target has no
>> OS version.
>>
>> There are two possible solutions here:
>> 1) You can still use -target with -mios-simulator-version-min as before but
>> you'd have to use '-target=x86_64-apple-darwin' to ensure that the iOS
>> version specified by '-mios-simulator-version-min' is used.
>> 2) I also do have a patch that implements the logic that you propose (use
>> the OS version in -m-version-min flag if target has none). If you
>> believe that the first solution is not suitable for your code then I can
>> commit it. At the same time I believe that we would rather not use this
>> patch, but if it's urgent for your projects then maybe I can land it now and
>> then we can establish some sort of timeline for when it can be reverted?
>>
>> Thanks,
>> Alex
>>
>>
>>> On 21 December 2017 at 08:00, James Y Knight wrote:
>>> I think if a version number isn't explicitly specified in the -target
>>> value, the value from -m-version-min ought to still be used, as
>>> it was before.
>>>
>>> Currently, clang will ignore the -m-version-min version number if
>>> the target has a particular OS specified, even if it has no version number
>>> as part of it.
>>>
>>> (We should be able to workaround this change backwards-compatibly by
>>> specifying in both the -target argument and in the -m-version-min
>>> arguments, but I do think the behavior should be fixed.)
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [libcxx] r276238 - Implement std::string_view as described in http://wg21.link/P0254R1. Reviewed as https://reviews.llvm.org/D21459
> On Jun 15, 2017, at 22:22, Eric Fiselier wrote:
>
>
>
>> On Thu, Jun 15, 2017 at 11:00 PM, Duncan P. N. Exon Smith
>> wrote:
>> Your suggestion is essentially to replace experimental/string_view with
>> something like:
>>
>> namespace std { inline namespace __1 { namespace experimental {
>> template
>> using basic_string_view = _VSTD::basic_string_view;
>> }}}
>>
>> That breaks:
>> 1. User compiles 1.cpp with older toolchain. 1.cpp implements
>> foo(std::experimental::string_view).
>> 2. User compiles 2.cpp with newer toolchain. 2.cpp calls
>> foo(std::experimental::string_view).
>> 3. User links 1.o with 2.o.
>>
>> I'm not sure if this matters.
>
> It can't matter. are allowed to break both their API and
> ABI as needed.
>
> Also I was suggesting
>
>namespace std { namespace experimental {
> using std::basic_string_view;
> using std::string_view;
> }}
>
> This approach will break code that expects experimental::string_view and
> std::string_view are different types:
> Example:
>
> void foo(std::string_view);
> void foo(std::experimental::string_view);
> foo(std::string_view{}); // ambiguous
>
>>
>>> On Jun 15, 2017, at 21:55, Eric Fiselier wrote:
>>>
>>> I would also want to do serious performance analysis on this patch. Does
>>> removing the string_view overloads cause less optimal overloads to be
>>> chosen? Perhaps allocating ones?
>>> That would be really unfortunate, and I'm not sure that's in the best
>>> interest of our users at large.
>>
>> Less optimal compared to what? C++17 code?
>
> Not sure yet, I'm trying to figure out what types the `const Tp&` overloads
> are attempting to soak up. Is it only string_view?
The type trait restricts it to things convertible to string_view that are not
const char *.
>
>
>>
>>> /Eric
>>>
On Thu, Jun 15, 2017 at 10:51 PM, Duncan P. N. Exon Smith
wrote:
> On Jun 15, 2017, at 19:42, Eric Fiselier wrote:
>
>
>
> On Thu, Jun 15, 2017 at 8:38 PM, Duncan P. N. Exon Smith
> wrote:
>> I just started working on a patch to add #if guards, and the first
>> interesting thing I found was the basic_string constructor:
>>
>>> template
>>> template
>>> basic_string<_CharT, _Traits, _Allocator>::basic_string(
>>> const _Tp& __t, size_type __pos, size_type __n, const
>>> allocator_type& __a,
>>> typename
>>> enable_if<__can_be_converted_to_string_view<_CharT, _Traits,
>>> _Tp>::value, void>::type *)
>>> : __r_(__second_tag(), __a)
>>> {
>>> __self_view __sv = __self_view(__t).substr(__pos, __n);
>>> __init(__sv.data(), __sv.size());
>>> #if _LIBCPP_DEBUG_LEVEL >= 2
>>> __get_db()->__insert_c(this);
>>> #endif
>>> }
>>
>
> That constructor was added in C++17, so removing it along with
> string_view should be OK.
> Assuming we don't use it to implement older constructors using a single
> template.
>
>
>> I suppose the decision was made so that std::string could take advantage
>> of it.
>>
>> Is it a conforming extension?
>
> No, because it can change the meaning of otherwise well defined code, as
> you pointed out initially.
Let me know if this patch is along the right lines. If so, I'll finish it
up and put it on phab.
experimental/filesystem/path.cpp doesn't compile, since
experimental/filesystem uses things like operator+=(string, string_view)
extensively. But I'd like an early opinion on the approach before I dig
in.
In string, the only function that needed to be rewritten was
string::compare(size, size, string, size, size). I'm nervous that
filesystem will be a bigger job.
>
>>
>>> On Jun 15, 2017, at 18:35, Eric Fiselier wrote:
>>>
>>> It *shouldn't* include , that's a given.
>>>
>>> IIRC, and Marshall would know better, I believe it was untenable to
>>> maintain a version of that didn't depend on
>>> after making
>>> the changes required for C++17.
>>>
>>> However inspecting now it does seem possible that the
>>> entanglement
>>> is avoidable.Though it's also likely I'm just not seeing the whole
>>> picture.
>>>
>>> /Eric
>>>
>>> On Thu, Jun 15, 2017 at 6:42 PM, Duncan P. N. Exon Smith
>>> wrote:
> On Jul 20, 2016, at 22:31, Marshall Clow via cfe-commits
> wrote:
>
> Modified: libcxx/trunk/include/string
> URL:
> http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/string?rev=276238&r1=276237&r2=276238&view=diff
> ==
>
> @@ -435,6 +461,7 @@ basic_string operator "" s( co
>>
Re: [PATCH] D30009: Add support for '#pragma clang attribute'
> On Apr 5, 2017, at 05:13, Aaron Ballman via Phabricator > wrote: > > aaron.ballman added inline comments. > > > > Comment at: lib/Sema/SemaAttr.cpp:578 > +return; > + Diag(PragmaAttributeStack.back().Loc, > diag::warn_pragm_attribute_no_pop_eof); > +} > > arphaman wrote: >> aaron.ballman wrote: >>> Perhaps adding a FixIt here would be a kindness? >> Where would the fix-it point to? I think only the user will know the >> location at which they meant to insert `#pragma clang attribute pop`. > Given that it's a warning rather than an error, and our recovery mechanism is > to effectively pop the pragma at the end of the TU, I was thinking it could > be added at the end of the TU. However, you are correct that it's probably > not where the programmer needs it, What about at the end of the file the push is in? This is likely to be used in header files, and it's probably unintentional if it extends past the end of the file of the push. I think we should consider the same thing for #pragma pack (although that's a little off-topic). > so a FixIt likely isn't appropriate. Does that suggest the warning should be > an error instead? Maybe it should be an error, but I still think a fixit would be nice if we can find a spot. > > > Repository: > rL LLVM > > https://reviews.llvm.org/D30009 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27387: [libc++] Add a key function for bad_function_call
I haven't looked at the patch, but yes, many developers on our platform back-deploy to older OS versions (and we support that via Clang flags, e.g., -miphoneos-version-min=8.0). They always build against the newest SDK/headers. -- dpnes > On Dec 5, 2016, at 00:35, Eric Fiselier via Phabricator > wrote: > > EricWF added a reviewer: dexonsmith. > EricWF added a subscriber: dexonsmith. > EricWF added a comment. > > In https://reviews.llvm.org/D27387#613071, @smeenai wrote: > >> In https://reviews.llvm.org/D27387#612975, @EricWF wrote: >> >>> I wonder if we should consider this a breaking ABI change and control it >>> using a `_LIBCPP_ABI` macro. @mclow.lists thoughts? >> >> >> This is forward-compatible (as in clients built against an older libc++ >> would be happy with this version) but not backwards-compatible (as in >> clients built against this version would not be able to run against an older >> libc++). Has libc++ been aiming to maintain compatibility in both directions? > > > Hmm, I'm not exactly sure. We don't make backward incompatible changes to > existing code often. I wonder if vendors like Apple require such backwards > compatibility. Maybe @dexonsmith can weigh in? > > > https://reviews.llvm.org/D27387 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [libcxx] r263036 - Implement LWG#2583: There is no way to supply an allocator for basic_string(str, pos)
There's a longer term fix involving availability attributes. My patch to the
test system for deployment targets is holding up a series of patches that tell
the compiler when API was introduced (with a 'strict' flag). This moves link
time errors (and load time errors when back deploying) to compile time. (I
also have patches to get the test suite green for all Apple deployment targets.)
That doesn't handle this case explicitly, but I have an idea for new support in
clang via a new flag:
- extern=10.10: only respect extern template for this function for dylibs 10.10
and later, otherwise leave as linkonce.
This would allow libc++ to add new basic_string functions to the dylib over
time.
-- dpnes
> On Mar 11, 2016, at 7:32 AM, Nico Weber wrote:
>
> I reverted this in 263246 for now. I think the right fix is just to add
> _LIBCPP_INLINE_VISIBILITY to the new constructor, but I'm not 100% sure.
>
>> On Thu, Mar 10, 2016 at 10:21 AM, Nico Weber wrote:
>> I think this is ABI-breaking. On OS X, new libc++ headers must work with old
>> system libc++.dylibs. When I build in this setup with this change, I get
>>
>> Undefined symbols for architecture x86_64:
>> "std::__1::basic_string,
>> std::__1::allocator >::basic_string(std::__1::basic_string> std::__1::char_traits, std::__1::allocator > const&, unsigned
>> long, std::__1::allocator const&)", referenced from:
>>
>> The new function probably needs that always_inline treatment that many other
>> functions have?
>>
>>
>>> On Wed, Mar 9, 2016 at 12:51 PM, Marshall Clow via cfe-commits
>>> wrote:
>>> Author: marshall
>>> Date: Wed Mar 9 11:51:43 2016
>>> New Revision: 263036
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=263036&view=rev
>>> Log:
>>> Implement LWG#2583: There is no way to supply an allocator for
>>> basic_string(str, pos)
>>>
>>> Modified:
>>> libcxx/trunk/include/string
>>> libcxx/trunk/test/std/strings/basic.string/string.cons/substr.pass.cpp
>>> libcxx/trunk/www/cxx1z_status.html
>>>
>>> Modified: libcxx/trunk/include/string
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/string?rev=263036&r1=263035&r2=263036&view=diff
>>> ==
>>> --- libcxx/trunk/include/string (original)
>>> +++ libcxx/trunk/include/string Wed Mar 9 11:51:43 2016
>>> @@ -98,8 +98,10 @@ public:
>>> basic_string(const basic_string& str);
>>> basic_string(basic_string&& str)
>>> noexcept(is_nothrow_move_constructible::value);
>>> -basic_string(const basic_string& str, size_type pos, size_type n =
>>> npos,
>>> +basic_string(const basic_string& str, size_type pos,
>>> // LWG#2583
>>> const allocator_type& a = allocator_type());
>>> +basic_string(const basic_string& str, size_type pos, size_type n,
>>> // LWG#2583
>>> + const Allocator& a = Allocator());
>>> basic_string(const value_type* s, const allocator_type& a =
>>> allocator_type());
>>> basic_string(const value_type* s, size_type n, const allocator_type& a
>>> = allocator_type());
>>> basic_string(size_type n, value_type c, const allocator_type& a =
>>> allocator_type());
>>> @@ -1397,7 +1399,9 @@ public:
>>> basic_string(size_type __n, value_type __c);
>>> _LIBCPP_INLINE_VISIBILITY
>>> basic_string(size_type __n, value_type __c, const allocator_type& __a);
>>> -basic_string(const basic_string& __str, size_type __pos, size_type __n
>>> = npos,
>>> +basic_string(const basic_string& __str, size_type __pos, size_type __n,
>>> + const allocator_type& __a = allocator_type());
>>> +basic_string(const basic_string& __str, size_type __pos,
>>> const allocator_type& __a = allocator_type());
>>> template
>>> _LIBCPP_INLINE_VISIBILITY
>>> @@ -2220,6 +2224,20 @@ basic_string<_CharT, _Traits, _Allocator
>>> #if _LIBCPP_DEBUG_LEVEL >= 2
>>> __get_db()->__insert_c(this);
>>> #endif
>>> +}
>>> +
>>> +template
>>> +basic_string<_CharT, _Traits, _Allocator>::basic_string(const
>>> basic_string& __str, size_type __pos,
>>> +const
>>> allocator_type& __a)
>>> +: __r_(__a)
>>> +{
>>> +size_type __str_sz = __str.size();
>>> +if (__pos > __str_sz)
>>> +this->__throw_out_of_range();
>>> +__init(__str.data() + __pos, __str_sz - __pos);
>>> +#if _LIBCPP_DEBUG_LEVEL >= 2
>>> +__get_db()->__insert_c(this);
>>> +#endif
>>> }
>>>
>>> template
>>>
>>> Modified:
>>> libcxx/trunk/test/std/strings/basic.string/string.cons/substr.pass.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/strings/basic.string/string.cons/substr.pass.cpp?rev=263036&r1=263035&r2=263036&view=diff
>>> ==
>>> --- libcxx/trunk/test/std/strings/basic.string/st
