Introduce basic linting infrastructure#34
Conversation
| struct identity { | ||
| // Returns `t`. | ||
| template <class T> | ||
| #if defined(__cpp_constexpr) |
There was a problem hiding this comment.
I realize this is just a formatting PR, but I'm wondering if we actually need this macro for Beman? This would imply support for something prior to c++11 which really isn't our aim here.
There was a problem hiding this comment.
Yes this is a formatting pr! I am just presenting a preview of the formatting that is going to be adopted.
This is a good point! There's also a macro test for __cpp_lib_transparent_operators at testing implying C++14.
There was a problem hiding this comment.
yes, well I guess I could see some library want c++14 support, but 98 -- not really. I suggest we move the macro at 33 as a drive by fix on this PR.
There was a problem hiding this comment.
That's fair! After the weekly sync yesterday I think I will expand the scope of this pr out to be "Introduce general pre-commit + linting", haven't got around to updating the title and description, so I will need to work on this for a bit, I will open a separate pr specifically deleting this line.
There was a problem hiding this comment.
Note that this implies C++14 as std::forward is constexpr after C++14 and (I don't think) constexpr can call non-constexpr function in 11.
There was a problem hiding this comment.
As another note you'll see in optional26 PR the unadorned use of constexpr https://github.com/beman-project/optional26/pull/64/files
There was a problem hiding this comment.
unadorned, cool new vocab for me!
I think I will actually hold creating a pr regarding this in hope of clang-tidy flagging it.
wusatosi
changed the title
|
Besides updating README, all configuration should be done. Opening for comments and reviews. |
wusatosi
requested review from
bretbrownjr,
camio,
dietmarkuehl,
neatudarius and
steve-downey
as code owners
camio
left a comment
camio
left a comment
There was a problem hiding this comment.
This PR looks great to me.
I've used markdownlint-cli in the past and it worked well. It's the first time I've used gersemi to format CMake and while the output was surprising in a couple instances, the fact it is automated overshadows this.
I think this should be merged and our style czar @JeffGarland can provide an improved .clang-format later.
Our README.md should include instructions on pre-commit, but it should be clear that this is only for developers intending to make changes to this repository (those building for use elsewhere don't need it). The Carbon pre-commit instructions are fairly decent.
@wusatosi, let me know if you want me to merge this now (and leave documentation to another PR) or if I should hold off.
Co-authored-by: David Sankel <[email protected]>
This comment has been minimized.
This comment has been minimized.
steve-downey
left a comment
steve-downey
left a comment
There was a problem hiding this comment.
👍
And I'd like to apply some of this to optional26 as well, soon.
|
GitHub was being GitHub... |
|
I don't think pre-commit's action's caching is working. |
5 participants
Introduce pre-commit based linting infrastructure:
clang-format) and does not pollute global scope, thus no virtual environment facing files (likerequirements.txt) is included. This is tested with an empty GitHub Codespace.Action Items:
.clang-formator equivalent #29 .setup clang-tidy linting: may be out of scope, see discussion below: Introduce basic linting infrastructure #34 (comment)gersemi, see: f3557fc for format diff)markdown-cli, see: 1ca6338 for format diff)Reference implementation: bemanproject/optional#52
.clang-formatis copied from Optional 26 & net29 with no modification.Closes: #29 .