RWTH Aachen University
- User Since
- Apr 2 2015, 4:52 AM (250 w, 3 d)
Thu, Jan 16
Wed, Jan 15
Tue, Jan 14
This revision was not accepted before being committed!
Mon, Jan 6
Can you please also submit a patch to libc++ where we copied this code from?
Tue, Dec 31
Mon, Dec 30
Yes, this must not be changed for compatibility reasons. If this really brings a benefit, we need new entry functions and update the compiler(s).
Dec 11 2019
Dec 7 2019
Dec 6 2019
Dec 2 2019
Nov 29 2019
Nov 19 2019
Random drive-by comments to parts of the patch, I won't review in full.
Nov 16 2019
Testing of openmp is currently very lacking, but I don't think this change will apply cleanly anymore...
Going through my old revisions, is this still something that we want?
I won't continue work on this.
Nov 5 2019
Heads up that this change broke compatibility with old binaries. Not sure if that is something worth to fix, but it should probably at least be spelt out somewhere...
Sep 28 2019
Sep 11 2019
The new test is failing for me because CHECK1 is not satisfied. Instead the line says MERGE-OUTER: 3 new files with 12 new features added; 11 new coverage edges (instead of 11 new features). I'm currently investigating what's wrong here, let me know if you have an idea.
LG iff the following is correct: The leak is when dyn_cast can't cast and returns null, right? If so, please add into the summary (and do so for future revisions, it will speed up review).
Sep 10 2019
The code itself looks good to me (it should probably be clang-formated). If others have comments, please raise them now.
Sep 8 2019
For the record, I don't think this would have worked: I do have libstdc++ available on my system (I just choose SANITIZER_CXX_ABI=libcxxabi for other reasons) and passing -stdlib=libstdc++ when building the test doesn't help as I've already said in D67298. Moreover, there's nothing in the test that requires libstdc++, it just happens to pass due to many chained implications, and restricting a test without understanding the root cause is probably no good idea.
Sep 7 2019
I think I just figured out that the original problem is about static runtime, so it doesn't make sense to run the test with dynamic asan.
Sep 6 2019
I've posted a change that makes the test work for me in D67298, please take a look.
Sep 4 2019
Also, there should be a summary of the changes in here.
Sep 3 2019
Sep 2 2019
Aug 31 2019
This changes error messages, so I'd say it's not NFC.
Aug 30 2019
Aug 15 2019
LG, thanks for the changes.
Please submit the test changes unrelated to the code changes in a separate patch!
The code changes look good to me, but the test doesn't pass on x86. We've faced the same problem when clang-offload-bundler was initially committed and the current testing is the best we were able to do.
Can we close this after D65341 landed?
Aug 14 2019
Okay, so I wasn't happy with the current explanations because I don't like "fixing" an issue without understanding the problem. Here's a small reproducer:
$ cat main.cpp #include "test.h"
Jon, please subscribe to openmp-commits so that commit emails get through immediately. Thanks!
Aug 13 2019
Change check_c_compiler_flag() to check_cxx_compiler_flag().
Aug 12 2019
Aug 11 2019
Remove compiler check for -Wno-uninitialized.
Remove another unnecessary cast.
Aug 10 2019
This test is also failing for me on CentOS 7, Intel x86. Any clues what's wrong here?
Aug 9 2019
LG after addressing the last minor nits (comment + cast + check).
Can you please rebase on top of D66019? Sorry, that'll probably give you some conflicts :-/