Closed lord-ne closed 2 years ago
Instead of annotating every functions, I think you can put the whole delay_cycles behind the config in lib.rs
. And add if cfg()
within the wrapper functions. Seems easier to maintain. And as a bonus, when you are reading the doc and clicking the src
button, the conditional compilation is obvious without having to dive into the implementation details.
FWIW:
#[cfg...
If we want to avoid having many cfg statements, there's a couple approaches we can take (that I can think of):
delay_cycles
module with a dummy module that just exposed a Delayer
with an empty delay_impl
asm!
macro with some custom macro (say we call it asm_if_avr!
) that is equal to the normal asm!
macro if compiling for avr, otherwise it does nothing.Out of these, I think the second one is probably better, since I'm not really a fan of having dummy code. But the second one is basically the same as what I already suggested, which is basically annotating each asm!
with a cfg
condition
In both case you are have dummy code. Your first option is close to what I proposed though. Put the module behind conditional. Have all public functions contain a conditional. Best of both worlds.
Except then I feel like we still have "many cfg
s". Basically, your idea is to have cfg
conditions wherever we use Delayer
, and my original idea was to have cfg
conditions every time we use asm!
. I don't see that big of a difference between the two tbh, but I'm fine with either
Sure if you count the number of if cfg
, it's both around 5 or 6. And I agree there is not much difference on this count.
The real difference in my opinion, is that one is within the Delayer module, while the other is at the interface between the module and the public API.
This means that within the module, any conditional compilation would be specific to some flavor of AVR. For example to support CPUs without sbiw
.
And on the other hand, the public functions are the one with the static conditionals. Keeping the concerns of non-avr compatibility in lib.rs
.
Conceptually I do still see some advantages to putting the cfg on the asm! blocks. Mainly because that way the code we're annotating is the exact code that doesn't compile.
But overall I think you're right. I don't have access to my computer for the next 20 hours or so (typing this on my phone), so if you want to make a new PR go ahead, otherwise I'll do it in a day or two
I know what you mean. You are hoping this way, most of the code is always type checked. And I agree this is important when the conditionals are for small differences. Like between AVR flavors. Not for complete behavioral changes like this is the case here.
There is no urgency for me. I am a contributor like you. Take care of this whenever you want.
The PR has been updated
Only compile asm! blocks when compiling for AVR targets. This allows the crate to be compiled for non-AVR targets (although it won't do anything). This could be useful, for example, if this crate or a crate that depends on it wants to run tests that run on the host machine (the machine doing the compilation).