bcraig added a comment.
Let's talk about these use cases.
> On the one hand, I think many users will want a libc that targets their
> particular freestanding environment so that they get the best performance
> characteristics (and other considerations) for their target.
I think this fits
bcraig added a comment.
> 1. A normal operating system target with a "hosted" libc.
> 2. A baremetal target with an "embedded" libc; basically a stripped down libc
> which only includes stuff that doesn't require an operating system. What
> exactly this includes varies; may include some form
bcraig added a comment.
> Okay, so this is potentially ignorance on my part. I was under the impression
> that folks using freestanding mode did not want any library to be linked in.
> Are there freestanding libc implementations out there that the user would
> link in instead that we defer to
bcraig added a comment.
> Okay, so this is potentially ignorance on my part. I was under the impression
> that folks using freestanding mode did not want any library to be linked in.
> Are there freestanding libc implementations out there that the user would
> link in instead that we defer to
bcraig added a comment.
A freestanding implementation doesn't necessarily mean that everything is
header-only. It's fine to require linking against a (freestanding) C runtime
library. All header-only is fine too though, if you want to make that work.
Architecturally, I don't feel it is
bcraig added a comment.
add a comment, and it will be fine in my eyes. You'll need signoff from the
code owner though.
Comment at: test/Analysis/padding_cpp.cpp:204-212
+// expected-warning@+1{{Excessive padding in 'struct FakeIntSandwich'}}
+struct FakeIntSandwich {
+ char
bcraig added inline comments.
Comment at: test/Analysis/padding_cpp.cpp:204-212
+// expected-warning@+1{{Excessive padding in 'struct FakeIntSandwich'}}
+struct FakeIntSandwich {
+ char c1;
+ int i;
+ char c2;
+};
+
Looking at this again... what new cases
bcraig added a comment.
You should probably add whoever the current code owner of that static analyzer
to this review. I have very little involvement with Clang these days, so a
"LGTM" from me doesn't carry much weight.
Comment at:
bcraig added a comment.
For those that would prefer random device to not exist if it isn't
cryptographically secure, please write a wg21 paper. I will gladly review such
a paper, and if you need a presenter, then I will present it if I am attending.
I won't be at Rapperswil, but I will be at
bcraig added a comment.
In https://reviews.llvm.org/D32411#1056381, @EricWF wrote:
> I spoke to STL on the MSVC team a while ago, and he stated that if we
> produced a paper describing why we need `#include_next` and the rational
> behind it, and they would pass that on to the front-end team.
bcraig updated this revision to Diff 126275.
bcraig added a comment.
Rebased
https://reviews.llvm.org/D32411
Files:
CMakeLists.txt
docs/DesignDocs/IncludeNextEmulation.rst
include/__config
include/__config_site.in
include/complex.h
include/cstddef
include/ctype.h
bcraig added a comment.
Talked with mclow and some Microsoft devs in Albuquerque. I'm not expecting
include_next to show up in MSVC. Sometime in the next month I hope to rebase
this change.
https://reviews.llvm.org/D32411
___
cfe-commits
bcraig added a comment.
In https://reviews.llvm.org/D37677#866743, @Quuxplusone wrote:
> This current patch just swaps out std::mutex for a std::mutex-alike class
> that claims to be faster for uncontested accesses. Definitely safer than my
> interpretation. :) If this patch actually helps,
bcraig added a comment.
Is there a benchmark where this demonstrates some performance improvement? I
fear that the switch to condition_variable_any will swamp any performance gains
from the switch to a spin lock.
Also, the spin lock is being held during allocating operations (the exception
bcraig added a comment.
LGTM. You should probably get one other person to approve though. I'm hoping
that @EricWF or @mclow.lists will take a look
https://reviews.llvm.org/D36423
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
bcraig added a comment.
The test headers should not be in the production include folder. They should
probably be in the benchmark folder.
If possible, follow the style and conventions of the existing tests. When you
can't follow the style and convention of the existing tests, try to make it
bcraig added a comment.
ping
https://reviews.llvm.org/D32411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig added a comment.
ping
https://reviews.llvm.org/D34170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig added a comment.
If you want the performance improvements in the BM_sort_std_worst_quick cases
preserved, you really need to port the benchmark from Aditya's repo into the
libcxx benchmark code base. Otherwise, the next person that comes along to
improve std::sort performance may very
bcraig added a comment.
I like this change in general. Dinkumware has been using introsort for 10+
years, so I'm a bit surprised that libc++ wasn't already.
Comment at: include/algorithm:4208
+
+ // Threshold(or depth limit) for introsort is taken to be 2*log2(size)
+
bcraig added a comment.
Those are interesting (and useful) results... but they don't look like they
came from the same algorithms.bench.cpp that I'm looking at...
https://github.com/llvm-mirror/libcxx/blob/master/benchmarks/algorithms.bench.cpp
That being said, the benchmark there only does 1k
bcraig added a comment.
alternatively, you could report the comparison of the old code vs. the new code
with an existing benchmark, like benchmarks/algorithms.bench.cpp
https://reviews.llvm.org/D36423
___
cfe-commits mailing list
bcraig added a comment.
This patch needs benchmarks that demonstrate the performance changes.
https://reviews.llvm.org/D36423
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig added inline comments.
Comment at: include/__mutex_base:51
#ifndef _LIBCPP_CXX03_LANG
-constexpr mutex() = default;
+#ifdef __GLIBC__
+constexpr
smeenai wrote:
> EricWF wrote:
> > Limiting `constexpr` to GLIBC implementations only is incorrect;
bcraig added a comment.
ping
https://reviews.llvm.org/D34170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig added a comment.
ping
https://reviews.llvm.org/D32411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig updated this revision to Diff 105845.
bcraig edited the summary of this revision.
bcraig added a comment.
Switched to static consts
https://reviews.llvm.org/D35174
Files:
include/string
Index: include/string
===
---
bcraig created this revision.
When using LIBCXX_ABI_UNSTABLE=YES, clang-cl gave the following warning:
P:\llvm_master\src\llvm\projects\libcxx\include\string(683,51):
warning: enumerator value is not representable in the underlying type
'int' [-Wmicrosoft-enum-value]
Fixed by providing a
bcraig updated this revision to Diff 105268.
bcraig added a comment.
Rebased.
Separating out logging into it's own class.
Also tweaked the output slightly so that the language dialect under test shows
up as the first line of output.
https://reviews.llvm.org/D34170
Files:
test/lit.cfg
bcraig added a comment.
_LIBCPP_MS_CRT seems fine too.
https://reviews.llvm.org/D34588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig added a comment.
LGTM, but you should probably get approval from somebody a bit more senior with
the project.
Comment at: include/__config:234-235
+// a MS compatibility version is specified.
# ifndef __MINGW32__
-#define _LIBCPP_MSVCRT // Using Microsoft's C
bcraig added a comment.
ping
https://reviews.llvm.org/D32411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig added inline comments.
Comment at: include/__config:234-235
+// a MS compatibility version is specified.
# ifndef __MINGW32__
-#define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
+#ifdef _MSC_VER
+# define _LIBCPP_MSVCRT // Using Microsoft's C
bcraig added a comment.
ping
https://reviews.llvm.org/D32411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig abandoned this revision.
bcraig added a comment.
This is very stale at this point, and isn't blocking anything. Closing.
https://reviews.llvm.org/D20596
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
bcraig accepted this revision.
bcraig added a comment.
LGTM.
@waltl : Do you need me to submit the changes, or will you handle that?
https://reviews.llvm.org/D32146
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
bcraig created this revision.
format.py and config.py were routinely reaching into the innards of
compiler.py, then setting variables in a very gcc / clang-centric way. Now,
all the compiler specific code has been moved to compiler.py. These makes it
possible to (in theory) build with MSVC
bcraig updated this revision to Diff 100650.
bcraig edited the summary of this revision.
https://reviews.llvm.org/D32411
Files:
CMakeLists.txt
docs/DesignDocs/IncludeNextEmulation.rst
include/__config
include/__config_site.in
include/complex.h
include/cstddef
include/ctype.h
bcraig added a comment.
> Are you suggesting that libc++ should stop declaring/defining these
> functions, at least publically?
I think that we generally shouldn't be giving functions names that are already
claimed elsewhere (like mbsnrtowcs and wcsnrtombs). It is my opinion that
these
bcraig added inline comments.
Comment at: include/support/newlib/xlocale.h:20
+#if defined(__NEWLIB__) && (__NEWLIB__ == 2) \
+&& defined(__NEWLIB_MINOR__) && (__NEWLIB_MINOR__ >= 5) \
+&& (!defined(__POSIX_VISIBLE) || (__POSIX_VISIBLE < 200809))
orivej
bcraig added a comment.
> I noticed that the Windows STL headers have to do this dance with `new` (even
> though they do `(foo)(...)` for `min` and `max`). If we're going to need
> to guard against a bunch of macros I would like to use a single approach.
> Other than updating the `#if
bcraig added a comment.
I agree that we need to be precise in our platform detection macros. On
Windows, we currently need to worry about which compiler is being used, whether
we are targeting the mingw environment or the native (?) environment, and which
c-runtime is being used.
For my
bcraig added a comment.
I like the warning that you generate for min and max macros existing. Is the
push_macro / pop_macro the right way to go though? You could throw extra
parenthesis around the declaration and usages of min/max to avoid macro
expansion.
#define add(x,y) (x)-(y)
bcraig added a comment.
LGTM
https://reviews.llvm.org/D32988
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig added inline comments.
Comment at: include/__config:232-235
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
I can see this helping when we are build libc++, but I don't think it helps
client apps. They could have included windows.h before including our
bcraig added a comment.
Getting the test suite green sooner rather than later seems like a good reason
to temporarily pick a 1-3 year solution rather than a 5+ year solution. Also,
as you mention, this isn't all throw away work, so it's still progress.
So yeah, this is fine to go in as is.
bcraig added inline comments.
Comment at:
test/std/language.support/support.exception/except.nested/rethrow_if_nested.pass.cpp:12
+
+// This test fails due to a stack overflow
// XFAIL: LIBCXX-WINDOWS-FIXME
EricWF wrote:
> BillyONeal wrote:
> > FWIW it does
bcraig added a comment.
I'm still working on this.
Things I've learned: A clang install is bundled with re-implementations of
several of these headers. The default #include_next behavior ends up pulling
in lots of those.
I have been able to get things working with a VC include path and a
bcraig added a comment.
In https://reviews.llvm.org/D32411#735128, @halyavin wrote:
> BTW, the list of include files which are located in [PROGRAM_FILES]\Microsoft
> Visual Studio 14.0\VC\include directory is
>
> - stdbool.h
> - limits.h
> - stdint.h
> - setjmp.h
>
> The rest is in
bcraig created this revision.
Herald added a subscriber: mgorny.
Visual Studio 2015 and 2017 don't implement include_next, so we'll use a
combination of a computed include and a CMAKE input to make it work. Also,
retrofit all the existing invocations of #include_next that we could hit in a
bcraig updated this revision to Diff 95520.
bcraig added reviewers: jyknight, mclow.lists, EricWF.
bcraig added a subscriber: cfe-commits.
https://reviews.llvm.org/D32147
Files:
include/__bsd_locale_fallbacks.h
include/__config
Index: include/__config
bcraig created this revision.
newlib 2.5 added the locale management functions, and so our stub
__nop_locale_mgmt.h shouldn't be included, as it conflicts with newlibs
definitions. newlib 2.4 and earlier still need the header though.
Patch by Martin J. O'Riordan
52 matches
Mail list logo
