[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-11-07 Thread Aditya Kumar via cfe-commits
hiraditya abandoned this revision.
hiraditya added a comment.

In https://reviews.llvm.org/D25624#588748, @mehdi_amini wrote:

> I think @EricWF already committed the patch with just the inline keyword?


Yes he's done already. Closing this.
Thanks for your feedback.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-11-07 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

I think @EricWF already committed the patch with just the inline keyword?


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-11-07 Thread Aditya Kumar via cfe-commits
hiraditya added a comment.

In https://reviews.llvm.org/D25624#582756, @mehdi_amini wrote:

> In https://reviews.llvm.org/D25624#582752, @mehdi_amini wrote:
>
> > (The commit message is confusing, it mentions "Currently basic_string's 
> > destructor is not getting inlined. So adding 'inline' attribute to 
> > ~basic_string()", please add the quote of the standard and mention that it 
> > enables instantiation)
>
>
> (i.e. "inline" does not cause directly "inlining" here, this comes as a side 
> effect of just starting to emit it)


You are right. The problem was that the function definition wasn't getting 
generated in the translation unit as a result the inliner cannot inline. For 
some reason clang does not generate the definition of available_externally 
functions. Sorry for the confusion, this patch is sitting for a long time so I 
forgot about it. Just adding inline should also have the same effect. I can 
commit a separate patch with just the inline specifier if there is an agreement.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-31 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

In https://reviews.llvm.org/D25624#583883, @sebpop wrote:

> In https://reviews.llvm.org/D25624#583163, @mehdi_amini wrote:
>
> > I'd like to understand why only the destructor?
>
>
> We have committed a patch to inline the constructor of std::string.
>  Do you think we should inline some other functions?


I'd say all of them (unless there's a reason to not do) :)
So: default to the inline keyword, with possibility of opt-out on a case by 
case.

The only impact of the inline attribute, as I understand, is to make sure some 
IR is emitted. There won't be any codegen, and the compile-time impact should 
be reasonnable.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-31 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

In https://reviews.llvm.org/D25624#584095, @hiraditya wrote:

> In https://reviews.llvm.org/D25624#583167, @mehdi_amini wrote:
>
> > I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like 
> > to understand what was the baseline?
> >  Here we add *both* the inline keyword and the always_inline attribute. I'd 
> > like to know if there is a benchmarks that shows that always_inline is 
> > beneficial on top of the inline keyword. 
> >  If we need to add always_inline anywhere: this is likely an inliner 
> > heuristic failure and we should at minima track it as an example to improve 
> > it.
>
>
> I remmber that the constructor of std::string wouldn't inline without 
> always_inline attribute https://reviews.llvm.org/D22782


I don't see where is it mentioned in the revision you are linking, can you 
elaborate what I should look at?


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-31 Thread Aditya Kumar via cfe-commits
hiraditya added a comment.

In https://reviews.llvm.org/D25624#583167, @mehdi_amini wrote:

> I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like to 
> understand what was the baseline?
>  Here we add *both* the inline keyword and the always_inline attribute. I'd 
> like to know if there is a benchmarks that shows that always_inline is 
> beneficial on top of the inline keyword. 
>  If we need to add always_inline anywhere: this is likely an inliner 
> heuristic failure and we should at minima track it as an example to improve 
> it.


I remmber that the constructor of std::string wouldn't inline without 
always_inline attribute https://reviews.llvm.org/D22782


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-31 Thread Sebastian Pop via cfe-commits
sebpop added a comment.

In https://reviews.llvm.org/D25624#583163, @mehdi_amini wrote:

> I'd like to understand why only the destructor?


We have committed a patch to inline the constructor of std::string.
Do you think we should inline some other functions?

In https://reviews.llvm.org/D25624#583167, @mehdi_amini wrote:

> I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like to 
> understand what was the baseline?


We ran a proprietary benchmark compiled with clang + libc++ and compared 
against clang + libstdc++.
With this patch, libc++ is on par with libstdc++.

> Here we add *both* the inline keyword and the always_inline attribute. I'd 
> like to know if there is a benchmarks that shows that always_inline is 
> beneficial on top of the inline keyword. 
>  If we need to add always_inline anywhere: this is likely an inliner 
> heuristic failure and we should at minima track it as an example to improve 
> it.

This is a good observation: I am not sure we ran the benchmark without the 
always_inline attribute.
We will try again wihout that attribute and report whether it matters 
performance wise.
Thanks Mehdi for catching this!


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-30 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like to 
understand what was the baseline?
Here we add *both* the inline keyword and the always_inline attribute. I'd like 
to know if there is a benchmarks that shows that always_inline is beneficial on 
top of the inline keyword. 
If we need to add always_inline anywhere: this is likely an inliner heuristic 
failure and we should at minima track it as an example to improve it.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-30 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

Before this gets re-committed, I'd like to understand why only the destructor?


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

I believe the issue is _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY  itself. I 
asked here: 
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161024/175454.html


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

I see a decl:

  declare hidden void 
@_ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev(%"class.std::__1::basic_string"*)
 unnamed_addr #9 align 2

(Note the hidden which prevent from finding it in the dyib)

And a use (after inlining):

  %1024 = call i32 @__cxa_atexit(void (i8*)* bitcast (void 
(%"class.std::__1::basic_string"*)* 
@_ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev to void 
(i8*)*), i8* bitcast (%"class.std::__1::basic_string"* 
@_ZZN12_GLOBAL__N_13DFA16writeTableAndAPIERN4llvm11raw_ostreamERKNSt3__112basic_stringIcNS4_11char_traitsIcEENS4_9allocatorIcE13SentinelEntry
 to i8*), i8* nonnull @__dso_handle) #1




https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

FYI reproduced locally, and investigating now.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

In https://reviews.llvm.org/D25624#582752, @mehdi_amini wrote:

> (The commit message is confusing, it mentions "Currently basic_string's 
> destructor is not getting inlined. So adding 'inline' attribute to 
> ~basic_string()", please add the quote of the standard and mention that it 
> enables instantiation)


(i.e. "inline" does not cause directly "inlining" here, this comes as a side 
effect of just starting to emit it)


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

Trying to figure out: it seems you are trying to address what I reported here: 
https://llvm.org/bugs/show_bug.cgi?id=26498 ; right?

> Except for inline functions, declarations with types deduced
>  from their initializer or return value (7.1.6.4), const 
>  variables of literal types, variables of reference types, 
>  and class template specializations, explicit instantiation
>  declarations have the effect of suppressing the implicit
>  instantiation of the entity to which they refer. [ Note: 
>  The intent is that an inline function that is the subject
>  of an explicit instantiation declaration will still be
>  implicitly instantiated when odr-used (3.2) so that the 
>  body can be considered for inlining, but that no 
>  out-of-line copy of the inline function would be generated
>  in the translation unit. — end note ]

So the `_LIBCPP_EXTERN_TEMPLATE(class _LIBCPP_TYPE_VIS basic_string)` 
block the instantiation of any non-inline template function. 
The effect of the "inline" keyword is actually allowing clang to actually 
instantiate and IRGen the function, so that it is available for inlining.

This seems like a generally good problem to solve for every such function and 
not only the destructor (I mentioned __init() as well in PR26498).

(The commit message is confusing, it mentions "Currently basic_string's 
destructor is not getting inlined. So adding 'inline' attribute to 
~basic_string()", please add the quote of the standard and mention that it 
enables instantiation)


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Sebastian Pop via cfe-commits
sebpop added a comment.

In https://reviews.llvm.org/D25624#582731, @mehdi_amini wrote:

> I don't understand:
>
> 1. The motivation for this change


This is a change for performance: we have seen some benchmarks where inlining 
the string dtor brings performance up by 5%: from what I remember, there is a 
string ctor shortly followed by a dtor that could be optimized away if both the 
ctor and dtor are inlined.  We already committed the patch to do the ctor 
inlining, and that gave us some performance. This patch is what remains to get 
the rest of the performance by inlining the string dtor.

> 2. Why is this correct?

There seems to be a problem the patch uncovers, and we will investigate.
Thanks for pointing out this issue.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Mehdi AMINI via cfe-commits
mehdi_amini added a comment.

I don't understand:

1. The motivation for this change
2. Why is this correct?

Can we restart the discussion here (I'm reverting to fix the LTO build failure 
here: 
http://lab.llvm.org:8080/green/job/clang-stage2-configure-Rlto_build/10737/console
 )


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Aditya Kumar via cfe-commits
hiraditya added inline comments.



Comment at: libcxx/include/string:1837
 template 
+inline _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()

EricWF wrote:
> The attribute should appear on the first declaration not the out-of-line 
> definition. Feel free to commit after making that change.
Okay, thanks for the correction.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Eric Fiselier via cfe-commits
EricWF accepted this revision.
EricWF added inline comments.



Comment at: libcxx/include/string:1837
 template 
+inline _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()

The attribute should appear on the first declaration not the out-of-line 
definition. Feel free to commit after making that change.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Aditya Kumar via cfe-commits
hiraditya updated this revision to Diff 76240.

https://reviews.llvm.org/D25624

Files:
  libcxx/include/string


Index: libcxx/include/string
===
--- libcxx/include/string
+++ libcxx/include/string
@@ -1834,6 +1834,7 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+inline _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2


Index: libcxx/include/string
===
--- libcxx/include/string
+++ libcxx/include/string
@@ -1834,6 +1834,7 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+inline _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Sebastian Pop via cfe-commits
sebpop added a comment.

Other than removing that comment the patch looks good.
Thanks!




Comment at: libcxx/src/string.cpp:10
 
+// For keeping the definition of ~basic_string in this translation unit.
+#define _LIBCPP_BUILDING_STRING

Let's not add this comment here, as this define may be used in the future for 
other purposes.  Also a simple grep would give you the places where this is 
used.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Eric Fiselier via cfe-commits
EricWF added a comment.

Actually we don't want to copy `memory.cpp` in this case. Please use 
`_LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY`. There are only examples in 
`` you can follow. Make sure to put it on the in-class declaration.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Eric Fiselier via cfe-commits
EricWF added a comment.

Additionally _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY is documented here 
http://libcxx.llvm.org/docs/DesignDocs/VisibilityMacros.html.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-28 Thread Aditya Kumar via cfe-commits
hiraditya updated this revision to Diff 76192.
hiraditya added a comment.

Addressed Sebastian's comments.


https://reviews.llvm.org/D25624

Files:
  libcxx/include/string
  libcxx/src/string.cpp


Index: libcxx/src/string.cpp
===
--- libcxx/src/string.cpp
+++ libcxx/src/string.cpp
@@ -7,6 +7,9 @@
 //
 
//===--===//
 
+// For keeping the definition of ~basic_string in this translation unit.
+#define _LIBCPP_BUILDING_STRING
+
 #include "string"
 #include "cstdlib"
 #include "cwchar"
Index: libcxx/include/string
===
--- libcxx/include/string
+++ libcxx/include/string
@@ -1834,6 +1834,9 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+#ifndef _LIBCPP_BUILDING_STRING
+inline _LIBCPP_INLINE_VISIBILITY
+#endif
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2


Index: libcxx/src/string.cpp
===
--- libcxx/src/string.cpp
+++ libcxx/src/string.cpp
@@ -7,6 +7,9 @@
 //
 //===--===//
 
+// For keeping the definition of ~basic_string in this translation unit.
+#define _LIBCPP_BUILDING_STRING
+
 #include "string"
 #include "cstdlib"
 #include "cwchar"
Index: libcxx/include/string
===
--- libcxx/include/string
+++ libcxx/include/string
@@ -1834,6 +1834,9 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+#ifndef _LIBCPP_BUILDING_STRING
+inline _LIBCPP_INLINE_VISIBILITY
+#endif
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-27 Thread Sebastian Pop via cfe-commits
sebpop added inline comments.



Comment at: libcxx/include/string:1841
 template 
+inline _LIBCPP_HEADER_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()

and let's also use the define just here:

  #ifndef _LIBCPP_BUILDING_STRING
  inline _LIBCPP_INLINE_VISIBILITY
  #endif



Comment at: libcxx/src/string.cpp:10
 
+#define _LIBCPP_HEADER_INLINE_VISIBILITY
+

To be consistent with memory.cpp:

  #define _LIBCPP_BUILDING_MEMORY

let's rename the define as:

  #define _LIBCPP_BUILDING_STRING



https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-26 Thread Aditya Kumar via cfe-commits
hiraditya updated this revision to Diff 75915.
hiraditya added a comment.

Added macro to keep the definition of destructor of basic_string in string.cpp


https://reviews.llvm.org/D25624

Files:
  libcxx/include/string
  libcxx/src/string.cpp


Index: libcxx/src/string.cpp
===
--- libcxx/src/string.cpp
+++ libcxx/src/string.cpp
@@ -7,6 +7,8 @@
 //
 
//===--===//
 
+#define _LIBCPP_HEADER_INLINE_VISIBILITY
+
 #include "string"
 #include "cstdlib"
 #include "cwchar"
Index: libcxx/include/string
===
--- libcxx/include/string
+++ libcxx/include/string
@@ -11,6 +11,10 @@
 #ifndef _LIBCPP_STRING
 #define _LIBCPP_STRING
 
+#ifndef _LIBCPP_HEADER_INLINE_VISIBILITY
+#define _LIBCPP_HEADER_INLINE_VISIBILITY _LIBCPP_INLINE_VISIBILITY
+#endif
+
 /*
 string synopsis
 
@@ -1834,6 +1838,7 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+inline _LIBCPP_HEADER_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2


Index: libcxx/src/string.cpp
===
--- libcxx/src/string.cpp
+++ libcxx/src/string.cpp
@@ -7,6 +7,8 @@
 //
 //===--===//
 
+#define _LIBCPP_HEADER_INLINE_VISIBILITY
+
 #include "string"
 #include "cstdlib"
 #include "cwchar"
Index: libcxx/include/string
===
--- libcxx/include/string
+++ libcxx/include/string
@@ -11,6 +11,10 @@
 #ifndef _LIBCPP_STRING
 #define _LIBCPP_STRING
 
+#ifndef _LIBCPP_HEADER_INLINE_VISIBILITY
+#define _LIBCPP_HEADER_INLINE_VISIBILITY _LIBCPP_INLINE_VISIBILITY
+#endif
+
 /*
 string synopsis
 
@@ -1834,6 +1838,7 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+inline _LIBCPP_HEADER_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-19 Thread Marshall Clow via cfe-commits
mclow.lists added a comment.

This is the same problem as is facing https://reviews.llvm.org/D24991


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-19 Thread Marshall Clow via cfe-commits
mclow.lists added a comment.

I don't have a problem with this being marked as inline, as long as it doesn't 
disappear out of the dylib.

There *has* to be a version of `basic_string, 
Allocator>::~basic_string` in the dylib - existing applications depend 
upon it. (same for `wchar_t`).


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-14 Thread Sebastian Pop via cfe-commits
sebpop added a comment.

If I remember correctly, we pushed the fix after 3.9 was released.
Could you please explain the problem of requiring trunk LLVM to build trunk 
libcxx?


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-14 Thread Eric Fiselier via cfe-commits
EricWF added a comment.

Has the fix been merged into the 3.9 branch? Does re-adding this attribute mean 
that Clang 4.0 is required to build LLVM w/ libc++?


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-14 Thread Sebastian Pop via cfe-commits
sebpop accepted this revision.
sebpop added a reviewer: sebpop.
sebpop added a comment.
This revision is now accepted and ready to land.

This got approved in the past review.
Let's commit it now that the clang bug was fixed.
Thanks Aditya for keeping track of this.


https://reviews.llvm.org/D25624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D25624: Added 'inline' attribute to basic_string's destructor

2016-10-14 Thread Aditya Kumar via cfe-commits
hiraditya created this revision.
hiraditya added reviewers: mclow.lists, EricWF.
hiraditya added subscribers: laxmansole, sebpop, cfe-commits.

Author: laxmansole

  

Original Differential Revision: https://reviews.llvm.org/D22834

  

Posting the patch again as the bug https://llvm.org/bugs/show_bug.cgi?id=30341 
is fixed.

  

Currently basic_string's destructor is not getting inlined. So adding 'inline' 
attribute to ~basic_string().
Worked in collaboration with Aditya Kumar.

  

git-svn-id: https://llvm.org/svn/llvm-project/libcxx/trunk@280944 
91177308-0d34-0410-b5e6-96231b3b80d8


https://reviews.llvm.org/D25624

Files:
  include/string


Index: include/string
===
--- include/string
+++ include/string
@@ -1834,6 +1834,7 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+inline _LIBCPP_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2


Index: include/string
===
--- include/string
+++ include/string
@@ -1834,6 +1834,7 @@
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template 
+inline _LIBCPP_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits