Skip to content

Conversation

@scrc-cg-pr-from-issue
Copy link

This PR was automatically generated from issue #156.

Authored by: @felix91gr

Closes #156.

@netlify
Copy link

netlify bot commented Aug 23, 2025

Deploy Preview for scrc-coding-guidelines failed.

Name Link
🔨 Latest commit 724f698
🔍 Latest deploy log https://app.netlify.com/projects/scrc-coding-guidelines/deploys/68f6a6409dc727000721dbd7

@PLeVasseur
Copy link
Collaborator

@felix91gr -- could you add the new tags you're using in another PR?

Note that this fails:

3:49:45 PM: /opt/build/repo/src/coding-guidelines/expressions.rst:334: WARNING: Need could not be created: Tags {'maintainability', 'portability', 'surprising-behavior'} not in 'needs_tags'. [needs.create_need]

@felix91gr
Copy link
Collaborator

@PLeVasseur ohh! I see. The Form for creating a New Guideline Issue didn't mention that new tags needed to be added before using them. That's a bit of Contributor Experience we could improve in the Form

image

I'll be adding the new tags right away :)

@felix91gr
Copy link
Collaborator

GEH. The FLS spec file got us again! Hahahah. Okay, brb, will open a PR to update it.

@felix91gr
Copy link
Collaborator

@x0rw I'll need your help again QwQ

It seems like the table in the original issue (same for #181's original issue) isn't being picked up quite how sphinx would want it. Do you know what I should do?

The table looks like this:

  | **Compilation Mode** | **`0 <= M < N`** | **`M < 0`**         |     **`N <= M`**    |
  |:--------------------:|:----------------:|:---------------------:|:-------------------:|
  | Debug                | Shifts normally  | Panics              | Panics              |
  | Release              | Shifts normally  | Shifts by `M mod N` | Shifts by `M mod N` |

@felix91gr
Copy link
Collaborator

I've fixed the table manually.

There's a typo that CI complains about; #206 fixes it

@felix91gr felix91gr added coding guideline An issue related to a suggestion for a coding guideline chapter: expressions decidability: decidable A coding guideline which can be checked automatically category: required A coding guideline with category required labels Oct 4, 2025
Co-authored-by: felix91gr <[email protected]>

Note: tables were written by hand due to limitations in our automation
Copy link
Collaborator

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see this working with the manual table.

Content-wise this is good.

There's a couple of formatting snags I pointed out.


.. code-block:: rust
fn bad_shl(bits: u32, shift: i32) -> u32 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may be some missing braces here depending on what was intended.

One possibility: you could back the indentation up to the same level as this line for line 576 if the intent was to have the upper bit be stand-alone from the bottom part. That'd then mean the lower part should also be outdented to the same level of indentation.

The last 2 observations show how this addresses **Reason 2**.

.. code-block:: rust
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above applies down here too.

Note: this must be addressed at the automation level,
since said code blocks were generated from a GH issue
@PLeVasseur
Copy link
Collaborator

Opened #212 to track to resolution so that we can get this build to pass.

@PLeVasseur
Copy link
Collaborator

@felix91gr -- please rebase and push. Build should work now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: required A coding guideline with category required chapter: expressions coding guideline An issue related to a suggestion for a coding guideline decidability: decidable A coding guideline which can be checked automatically

Projects

None yet

4 participants