Add support for C preprocessor output#2538
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2538 +/- ##
==========================================
+ Coverage 71.10% 71.14% +0.04%
==========================================
Files 64 64
Lines 35487 35584 +97
==========================================
+ Hits 25232 25316 +84
- Misses 10255 10268 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AJIOB
force-pushed
the
support-preprocessed
branch
from
9f561eb to
8ed5ba5
Compare
ahartmetz
left a comment
ahartmetz
left a comment
There was a problem hiding this comment.
The commit message should either say "preprocessor output" or "preprocessed input". Preprocessor output is sccache input.
|
By the way, what's the use case for this? Would be nice to mention it somewhere. When I see this, I think "Huh, I guess this could make sense, but I've compiled a ton of software and never had a need for it". |
I have 2 reasons to do it:
The C preprocessed files can be easier verified, that the arch-dependent asm files. Are these reasons enough? |
Yes, thanks. I see the TODOs now. Would it be possible to add .s (and maybe .S, I know that one does need preprocessing) support in the same branch then, or is that a larger change that you don't want to make yet? |
|
I want to do it as a dedicated PR: need to test it additionally |
AJIOB
force-pushed
the
support-preprocessed
branch
from
c37dde7 to
9438cca
Compare
AJIOB
changed the title
AJIOB
force-pushed
the
support-preprocessed
branch
from
9438cca to
0292994
Compare
AJIOB
changed the title
AJIOB
force-pushed
the
support-preprocessed
branch
from
0292994 to
ff781bd
Compare
| preprocessor_result.stdout.len() | ||
| ); | ||
| (preprocessor_result.stdout, include_files) | ||
| } else { |
There was a problem hiding this comment.
You could move this to the beginning (followed by else if) and prevent a lot of "chaff" whitespace changes and a level of indentation. Yes it's not the common case, but OTOH, having a trivial implementation first is helpful to see what the full version is essentially doing.
There was a problem hiding this comment.
Do you suggest adding next parts duplication? Or just swap if & else parts between each other?
P.S. You can see the difference without spaces on the github UI
There was a problem hiding this comment.
Yeah, I could only understand what's going on well enough to suggest this after I found the option. (And git diff and git blame have it, too, TIL)
So anyway...
There was a problem hiding this comment.
What I meant was:
let (preprocessor_output, include_files) = if !needs_preprocessing {
//...
} else if let Some(preprocessor_key) = &preprocessor_key {
// ... everything else as before
Not sure if that syntax even works
|
This looks OK to me, but I feel too noob at Rust (or sccache for that matter) to approve it - at least for now. I'll change my mind if nobody else steps up for a several weeks. |
|
lgtm |
4 participants
No description provided.