Introduce basic linting infrastructure #34
Conversation
| @@ -27,16 +27,14 @@ namespace beman::exemplar { | |||
| struct __is_transparent; // not defined | |||
|
|
|||
| // A function object that returns its argument unchanged. | |||
| struct identity | |||
| { | |||
| 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.
There was a problem hiding this comment.
May be bundled into #38
wusatosi
changed the title
Current CI does not push format changes to pr/ main branch, I would be interested in putting auto clang-format in but that's out of the scope of this PR. |
|
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
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.
|
GitHub was being GitHub... |
|
I don't think pre-commit's action's caching is working. |
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: beman-project/optional26#52
.clang-formatis copied from Optional 26 & net29 with no modification.Closes: #29 .