- 
                Notifications
    
You must be signed in to change notification settings  - Fork 10.6k
 
[CMake] Simplify add_swift_library #19491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
           CC: @Rostepher @gottesmm  | 
    
| 
           @swift-ci please test  | 
    
| 
           Build failed  | 
    
| 
           Build failed  | 
    
52fab37    to
    ba77955      
    Compare
  
    | 
           @swift-ci please test  | 
    
| 
           Build failed  | 
    
| 
           Build failed  | 
    
Hoist out the some of the architecture independent variables outside of the inner loop. This should simplify the logic for the function and improve generation times. (NFC)
The entire if block is in the `SWIFTLIB_TARGET_LIBRARY` condition, so these checks add nothing but a level of indentation. (NFC)
ba77955    to
    05bf903      
    Compare
  
    | 
           @swift-ci please test  | 
    
| 
           CC: @Rostepher  | 
    
| 
           Build failed  | 
    
| 
           Build failed  | 
    
| 
           Bleh, the precondition check isnt working ... I'm not sure why; I guess we can deal with that simplification later.  | 
    
Merge two cases into a single case rather than keeping them separate. Additionally check the condition early rather than perform the action and then revert it. NFC.
05bf903    to
    7974e39      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick thing that I want verified by @zisko and @Rostepher. Beyond that, LGTM
| set(swiftlib_link_flags_all ${SWIFTLIB_LINK_FLAGS}) | ||
| if(${sdk} STREQUAL IOS_SIMULATOR) | ||
| if(${name} STREQUAL swiftMediaPlayer) | ||
| # message("DISABLING AUTOLINK FOR swiftMediaPlayer") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if you should comment this out. I think this caused problems in the past and the logging was meant to provide info if this happened.
@zisko @Rostepher is this useful for you guys? If not, we should just delete this line.
| 
           Actually, sorry just just read the first commit. Sorry! I need to review the rest.  | 
    
| 
           @swift-ci please test  | 
    
| 
           Build failed  | 
    
| 
           Build failed  | 
    
| 
           The rest of the patches look ok to me as well.  | 
    
Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.
Resolves SR-NNNN.