- User Since
- Dec 23 2015, 11:13 AM (186 w, 3 d)
Mon, Jun 24
Thanks for the review!
Apr 1 2019
I can't comment to much on the cmake as my cmake-fu is weak, but from what I can tell it looks good.
Jan 29 2019
Thank you for testing and your feedback!
Typo fixed, I only see one llvm-shlib/CMakeLists.txt file.
Ugh, I see some types, let me respin.
This should fix it, doing a build now.
Okay, I have updated the patch now with the suggestions from @stella.stamenova, thanks! Please try it out, I'm currently running a build myself using ninja to make sure that it still works.
Jan 25 2019
Thanks! As I don't have commit access I will need somebody to land this, thanks!
Jan 24 2019
Jan 22 2019
@serge-sans-paille I do not, so that would be greatly appreciated.
Jan 17 2019
Thanks, this should fix all concerns raised, I tested this as well and seems to work.
Jan 16 2019
@hans @serge-sans-paille So I built the package and has verified that the LLVM-C.dll is included in the package but I could not find the LLVM-C.lib file tho. After some hours of digging through the multitude of *.cmake files I have to admit defeat on how to get that in there, any pointers would be very much welcome.
Have now tested this patch and confirmed that it is working for me at least. Needed to make the libs argument optional or the python script would error out. I did not see any sign of $(Configuration) in my scripts.
Should address all of the comments, haven't done anything the $(Configuration) @hans saw in the error message. Well setup something to test this.
And remove the parentheses.
Not tested, will address last comment and then setup something I can test this on.
Updated changes with comments from @serge-sans-paille, thanks!
No I don't have commit access, so please feel free to commit. Will this land before the branch point and also make it out in that release?
Aug 27 2018
The C-API changes looks good to me and the logic makes sense. Just one nit.
Aug 22 2018
Aug 9 2018
This can be closed, as changes has landed in a different review. See D45100.
Aug 7 2018
Can this please please please be pushed ASAP so it can go in to the 7.0 release?
Aug 6 2018
This has is already in the C API now, so closing.
That should be all of the comments done.
Updated with @bhelyer changes and updated to latest master.
Jul 8 2018
Another update of the commit for llvm master.
Apr 6 2018
Added people who seems to be doing stuff with cmake on windows.
Updated the diff to master, haven't build tested yet.
Thanks for testing again @Memnarch, I'm going to update this changeset soon and then try to get it included in LLVM Weekly.
Oct 20 2017
This is a NFC update to the patch, bringing it up to master. As I wrote above I can't test it, but should work just fine.
No sorry, I moved and started a new job. And don't have access to the windows machine for a while.
Sep 21 2017
Sep 14 2017
I don't have commit access so I need somebody to commit this for me, it has been like a month since I got the first LGTM on this review.
Sep 7 2017
This should not have any other changes, I think. Problem is I do not know which revision the old one was based on.
I have updated the commit to use for locating the libs CMAKE_CFG_INTDIR and should fix the issue @Memnarch saw. I only have build tools installed so I don't know if I can test it under a Visual Studio like environment.
Aug 10 2017
Thanks for the review, I do not have commit access so somebody needs to commit this for me.
Jul 22 2017
Jul 17 2017
Jul 6 2017
Apr 23 2017
Hey cool that you are doing this work, finally!
Apr 10 2017
Feb 28 2017
Only spotted one case of a Value not being cloned. Others use DeclareBB.
I think that's the last of the changes needed for the echo.cpp files, can we agree on something for the LLVMBuildCallInPad function and have somebody that knows more of what the IR should look like comment on the IR file.
Last update from Bernard.
Feb 26 2017
Fixes for Core.[cpp|h] files.
Added the CloneValue call and it now passes the test again. I had to add
ConstantTokenNone and ConstantNullPointer to the clone_constant_imple
Feb 25 2017
Adding newline to end of file.
Updated diff from Bernard.
Feb 24 2017
Feb 15 2017
Aug 18 2016
This works for my test case as well, nice work! :D
Aug 16 2016
This commit compiles, fixes my test case and my full code now links correctly! Awesome thanks.
This change gives me linking errors when I try to compile it on git head.
Aug 3 2016
Jun 8 2016
I would like to see something that adds the integer and string attributes that @jyknight brought up, so we know they work before we commit to this API.
May 23 2016
Considering how long the other Attribute review is taking just remove the function all together, and put it in once we actually have the finished interface down.
Apr 28 2016
I kind of prefer the AttributeSet approach, but don't feel to strongly about it.
I strongly feel that we should just add new function and then remove the old ones the release after.
Apr 25 2016
So they are completely redoing the debug info interface which this is a precursor for, so I dunno how much benefit this will give us right now.
Apr 21 2016
I don't know that much about llvm internals, but this seems to make sense.
As I was reading this patch and comments I had some concerns about the different behavior between bfd and gold and that error messages would not provide enough information. The patch seem to address these concern so I'm happy with this patch.
Apr 19 2016
I thought we came to the conclusion that we should add new functions and not change ABI on the old ones. Yes this will mean copies of the functions but its only for a release.
Looks good, thanks!
Apr 17 2016
I'm not sure that this is worth the effort and code churn.
Maybe not the links but put in the comment that its function/arg/return attributes that this functions returns, and mention that they are listed in the language reference.
Apr 13 2016
About the code:
The function requires documentation. Is there a list of attributes somewhere? What does it return on failure? How long are the values returned valid? Per context, per thread, per process, per release?
Whelp that was a bit fast.
Apr 7 2016
Apr 5 2016
I want a second opinion on this issue.
Looks good, thanks for all the work!
Apr 4 2016
Will this for "uwtable" return the same value as LLVMAttribute.UWtable?
Apr 3 2016
You should put the unrelated commits in another one, they are good clean ups.
Can we please have one release with the new and old functions. Removing and adding replacement functions in the same release makes the upgrade path much harder. I can not upgrade the LLVM version I use because it breaks my build (build bots, other users and so on). And I can not change my code without breaking anything that still uses the old version (build bots, other users and so on). This is all made worse that the symbols are still there but their semantics have change, so it will just be hard to debug black magic errors.