search

quil-lang / quilc

The optimizing Quil compiler.
Apache License 2.0
452 stars 73 forks source link

Style warning cleanup #826

Open gefjon opened 2 years ago

gefjon commented 2 years ago

Quiet a few SBCL style warnings, mostly related to forward-referencing struct accessors that would otherwise be inlined.

gefjon commented 2 years ago

Also can you M-q the comment?

What line length do you prefer?

can we just move this whole rewiring section to be defined after token is?

My gut says no; token is defined in the parser, which sorta necessarily comes after the ast is defined. We could adopt a policy of moving all definitions earlier into a defs.lisp file.

Can we reverse it? Can we

(declaim inline) (defstruct) (declaim notinline)

and just inline whenever we feel we need to?

Adding bulky notinline decls everywhere just to quiet SBCL is sort of a code smell to me; I'd rather condense it all, and use inline to drive performance, not notinline to hush warnings.

No, I don't believe this works. If it did, the previous version that just notinline'd the struct accessors before definition would have prevented the warnings. Unfortunately, I think that the only ways to prevent these warnings are:

  1. Use ASDF's :around-compile to sb-ext:muffle-warnings.
  2. Break circular dependencies by moving structure definitions into a defs.lisp file.
  3. Complain to the SBCL devs to remove this style warning, because it's stupid.
  4. What I did.

I aimed for 4 because it seemed the least invasive fix, but 2 seems more correct. 1 feels like a gross hack, and 3 seems unlikely to work.

karlosz commented 2 years ago

If you just want SBCL to stop complaining about certain warnings, sb-ext:muffle-conditions does that for you.

I think adding inline/notinline to shut the compiler up would be poor style. Moving definitions around works if you want speed, as the style warnings note. Maybe those warnings should get downgraded to just compiler notes...

gefjon commented 2 years ago

Putting a "global" muffle-conditions (I mistakenly wrote muffle-warnings in my prev comment, but that's the declaration I meant) around ASDF compiling feels unfortunate, and putting locally muffle-conditions around callsites feels strictly worse than the locally notinlines because it would involve additional #+sbcl syntactic overhead.

If SBCL could be convinced to downgrade "unable to inline" from a style-warning to a note from a style-warning (either specifically for defstruct accessors because their inline declarations are implicit, or for all inline functions), I think that would be an improvement.

What should be a style-warning is using setf on a forward-referenced struct accessor, because the spec permits those to be implemented as setf-expanders rather than setf-functions. I don't believe QuilC does this, but the fact that I have to write about the distinction tells me that maybe we should move all our defstructs into an early defs.lisp file.

karlosz commented 2 years ago

What's wrong with wrapping muffle conditions with asdf?

The condition class for failure to inline is 'inlining-dependency-failure. You can use that to muffle of all inlining failure notes (and only those notes) if it's felt that all such failure notices are pointless.