Skip to content
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

Define code style #15

Open
burmako opened this issue Aug 17, 2022 · 10 comments · Fixed by #183
Open

Define code style #15

burmako opened this issue Aug 17, 2022 · 10 comments · Fixed by #183
Assignees
Labels

Comments

@burmako
Copy link
Contributor

burmako commented Aug 17, 2022

Currently, StableHLO doesn't have a formally-defined code style, but it will be good to have one. We could start with inheriting MLIR-HLO's code style to maximize familiarity for existing CHLO/MHLO users.

@burmako burmako self-assigned this Aug 17, 2022
@bhack
Copy link

bhack commented Aug 23, 2022

We will need a code style also for #32

@jpienaar
Copy link
Member

Note: CHLO/MHLO has mixed code style (which would be good to rectify). I'd say adopting either MLIR's style guide or Google's style guide (C++ side, Python side MLIR doesn't have en explicit one but close to Google's Python one in practice) would allow most familiarity. With the former being more consistent for end to end. (Builders and the like could also follow different style but that ends up being confusing).

@burmako
Copy link
Contributor Author

burmako commented Aug 26, 2022

Further questions to handle when defining code style, via jpienaar's review of a recent PR:

  • Use of doxygen and which constructs or conventions are followed (comment).
  • Error conventions (comment).

@burmako
Copy link
Contributor Author

burmako commented Sep 6, 2022

The last two weeks have been pretty hectic as we've been bootstrapping a GitHub-centric StableHLO development process from the Google-centric MLIR-HLO development process. During that time, we have set up the basic infrastructure (pull requests, issues, CI) and are almost done setting up regular integrates from StableHLO into MLIR-HLO.

I anticipate that we'll get to defining code style fairly soon (within the next 1-2 weeks).

In the meantime, @GleasonK has set up a CI check with runs git-clang-format (#49) and opened a ticket to also run clang-tidy likely bootstrapping it from MLIR-HLO (#61). In #76, @joker-eph has explored what it would take to switch the codebase from Google's clang-format style that our codebase currently follows to LLVM's clang-format style.

@burmako
Copy link
Contributor Author

burmako commented Sep 15, 2022

Another part of code style is Markdown formatting. Let's find a formatter / linter, so that we can automate changes like #128. (Also see Kevin's comment in that PR).

@sdasgup3
Copy link
Member

Regarding formatting markdowns: Adding links to the markdown might break the 80 lines constraint. The script can ignore those.

@burmako
Copy link
Contributor Author

burmako commented Sep 21, 2022

We're getting closer to the point when all the urgent work is done (e.g. our code now ships as part of MLIR-HLO which is great, but we still need an RFC process which is blocking quite a few things), at which point I'm planning to write something up about this.

One more item to comment on in the code style style is commit messages. Here's MLIR's guidance about this (thank you, Mehdi, for the link!): https://mlir.llvm.org/getting_started/Contributing/#commit-messages.

GleasonK added a commit that referenced this issue Sep 23, 2022
…ng-format (#183)

Two changes in this PR:

1) Refactor the lint action to be a locally runnable bash script. This should give _more_ predictable results in terms of addressing CI failures.

2) Update `.clang-format` to use Google style in the short term. More on this rationale below.

Background on all this is that my editor formatted a large chunk of a file which caused the lint action to fail remotely, and took me quite some time to roll back the individual formatting changes, since google styling appears to prefer the most common spacing of `Type* var` vs `Type *var` in a file as the proper formatting.

This change will allow local qualification / fixing, as well as provide a stop-gap on editor formatting issues while #15 is discussed. While I agree that we should resolve #15, I think more discussion is required, and there is unnecessary friction in the current setup/CI of clang-format in the current repo that should be addressed. Happy to drive the formatting conversation in the near future, but think this should be in place in the short term.

GH Actions run testing that it errors properly in CI: https://github.com/GleasonK/stablehlo/actions/runs/3106772722/jobs/5034066123

Closes #75
@burmako
Copy link
Contributor Author

burmako commented Sep 23, 2022

The linked pull request contributed to defining code style (or, more precisely, codifying a baseline that we'll build on), but this ticket is far from being resolved at this point :) Reopening.

@burmako burmako reopened this Sep 23, 2022
@GleasonK
Copy link
Member

Good catch. The phrasing "should resolve <issue> soon" from that PR falls under the words to link/close an issue.

@GleasonK
Copy link
Member

GleasonK commented Dec 20, 2022

Another part of code style is Markdown formatting.

@scottamain has been looking into this recently: #795, #792, #793, #729

@burmako burmako assigned GleasonK and unassigned burmako Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants