Skip to content

Commit c87885b

Browse files
authored
Lint script (#361)
## Changes * This closes the C++ Style Refactor project. * Resolves #316. * Resolves #317. * Resolves #319. * Closes #357. * Reformats all C++ code according to [Google C++ Style Guide]. * Adds `.clang-format` file specifying the exceptions to the Google style guide. * Adds lint.sh script which prints a diff generated by `clang-format` (if any) and the errors generated by `cpplint`, and allows one to apply formatting in place with `./lint.sh -i`. * Adds Lint step to CI before Build. * Updates .gitbub/CONTRIBUTING.md with instructions on how to use lint.sh. * Adds the missing test file to the Xcode project. ## Comments * Linting in CI is not parallelized and is using the same Docker image because it will eventually include linting of the Wolfram Language code as well as described in #247. * Bin packing is now disallowed in addition to the existing style exceptions. * All `cpplint` errors are fixed as well, not just the formatting. * `#include <chrono>` is not allowed by `cpplint` for Chromium-specific reasons, so added `// NOLINT` there. * Renamed `SetTest.cpp` -> `Set_test.cpp` because the suffix appears to be hard-coded to `cpplint`. * Before the formatting, the code was mostly following this style (imperfectly): ``` --- Language: Cpp BasedOnStyle: Google ColumnLimit: 120 BinPackArguments: false BinPackParameters: false AllowShortFunctionsOnASingleLine: Empty IndentWidth: 4 NamespaceIndentation: All AccessModifierOffset: -4 FixNamespaceComments: false SpaceAfterTemplateKeyword: false BreakConstructorInitializers: AfterColon SpacesBeforeTrailingComments: 1 ... ``` ## Examples * Break formatting/code style somewhere in the code, and run `./lint.sh`. For example, we can remove `explicit` on line 22 of Match.cpp, and add a spurious space on the line below: ``` > ./lint.sh --- libSetReplace/Match.cpp +++ formatted @@ -24 +24 @@ - bool operator()(const MatchPtr& a, const MatchPtr& b) const { + bool operator()(const MatchPtr& a, const MatchPtr& b) const { libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * We can run `./lint.sh -i` to fix the space automatically: ``` > ./lint.sh -i --- libSetReplace/Match.cpp +++ formatted @@ -24 +24 @@ - bool operator()(const MatchPtr& a, const MatchPtr& b) const { + bool operator()(const MatchPtr& a, const MatchPtr& b) const { libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * The output is the same, however, if we run `./lint.sh` again, there will be no diff: ``` > ./lint.sh libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * Finally, if we put explicit back in, there will be no output, which means everything is ok: ``` > ./lint.sh ```
1 parent e7f1d42 commit c87885b

File tree

16 files changed

+1480
-1421
lines changed

16 files changed

+1480
-1421
lines changed

.circleci/config.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ jobs:
1010
steps:
1111
- checkout
1212

13+
- run:
14+
name: Lint
15+
command: ./lint.sh
16+
1317
- run:
1418
name: Build
1519
command: ./build.wls

.clang-format

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
Language: Cpp
3+
BasedOnStyle: Google
4+
AllowShortIfStatementsOnASingleLine: Always
5+
BinPackArguments: false
6+
BinPackParameters: false
7+
ColumnLimit: 120
8+
DerivePointerAlignment: false
9+
Standard: c++17
10+
...

.github/CONTRIBUTING.md

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -347,33 +347,34 @@ In addition to that, here are some more-or-less established rules:
347347
* Start global constants with `$`, whether internal or public, and tags (such as used in [`Throw`](https://reference.wolfram.com/language/ref/Throw.html) or [`Sow`](https://reference.wolfram.com/language/ref/Sow.html), or as generic enum labels) with `$$`.
348348

349349
#### C++
350-
New code should follow [Google C++ Style](https://google.github.io/styleguide/cppguide.html) guidelines, save for
351-
the exceptions laid out in the list below. The code in this repository is currently undergoing refactoring to
352-
conform to this specification. You can track the progress of this refactoring
353-
[here](https://github.com/maxitg/SetReplace/projects/2).
354-
355-
The following are exceptions to Google C++ Style:
350+
The code should follow [Google C++ Style](https://google.github.io/styleguide/cppguide.html) guidelines, save for the
351+
following exceptions:
356352
* Maximum line length is 120 characters.
357353
* Function and variable names, including const and constexpr variables, use lower camel case.
358354
* Namespace and class names use upper camel case.
359355
* C++ exceptions may be thrown.
360-
* All indentation uses four spaces, except for access specifiers which uses one.
361356
* White space in pointer and reference declarations goes after the `*` or `&` character. For example:
362357
* `int* foo;`
363358
* `const std::string& string;`
359+
* If splitting function arguments into multiple lines, each argument should go on a separate line.
364360
* License, authors, and file descriptions should not be put at the top of files.
365361
* Doxygen is used for documentation.
362+
* We use `.cpp` and `.hpp` extensions for source files.
366363

367-
A useful tool for confirming your code's adherence to the above code style is to use
368-
[cpplint](https://raw.githubusercontent.com/google/styleguide/gh-pages/cpplint/cpplint.py). You can lint a C++ source
369-
or header file like this:
370-
```bash
371-
cpplint.py --linelength=120 --filter=-legal/copyright file_name
372-
```
373-
If cpplint flags a portion of your code, please make sure it is adhering to the proper code style. If it is a false
364+
We use [`clang-format`](https://clang.llvm.org/docs/ClangFormat.html) for formatting and
365+
[`cpplint`](https://raw.githubusercontent.com/google/styleguide/gh-pages/cpplint/cpplint.py) for linting.
366+
367+
To run these automatically, call `./lint.sh`. This will print a formatting diff and error messages from `cpplint`.
368+
If there are no errors found, it will exit with no output.
369+
To edit the code in place with the fixed formatting use `./lint.sh -i`.
370+
371+
If `cpplint` flags a portion of your code, please make sure it is adhering to the proper code style. If it is a false
374372
positive or if there is no reasonable way to avoid the flag, you may put `// NOLINT` at the end of the line if there is
375373
space, or `// NOLINTNEXTLINE` on a new line above if there is no space. For any usages of `// NOLINT` or
376374
`// NOLINTNEXTLINE`, please describe the reason for its inclusion both in a code comment and in your pull request's
377375
comments section.
378376

377+
If you want to disable formatting, use `// clang-format off` and `// clang-format on` around the manually formatted
378+
code.
379+
379380
That's all for our guidelines, now let's go figure out the fundamental theory of physics! :rocket:

SetReplace.xcodeproj/project.pbxproj

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
/* Begin PBXFileReference section */
2323
6909EBA922258611001458D2 /* Match.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = Match.cpp; sourceTree = "<group>"; };
2424
6909EBAA22258611001458D2 /* Match.hpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = Match.hpp; sourceTree = "<group>"; };
25+
691E077A2471CED500D2BDD5 /* CMakeLists.txt */ = {isa = PBXFileReference; lastKnownFileType = text; path = CMakeLists.txt; sourceTree = "<group>"; };
26+
691E077C2471D1DD00D2BDD5 /* Set_test.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = Set_test.cpp; sourceTree = "<group>"; };
2527
695F486E222443F20058E057 /* Set.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = Set.cpp; sourceTree = "<group>"; };
2628
695F486F222443F20058E057 /* Set.hpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = Set.hpp; sourceTree = "<group>"; };
2729
6961A1D523184ABC009461C8 /* Expression.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = Expression.cpp; sourceTree = "<group>"; };
@@ -44,6 +46,15 @@
4446
/* End PBXFrameworksBuildPhase section */
4547

4648
/* Begin PBXGroup section */
49+
691E07792471CED500D2BDD5 /* test */ = {
50+
isa = PBXGroup;
51+
children = (
52+
691E077A2471CED500D2BDD5 /* CMakeLists.txt */,
53+
691E077C2471D1DD00D2BDD5 /* Set_test.cpp */,
54+
);
55+
path = test;
56+
sourceTree = "<group>";
57+
};
4758
695F485A222440B90058E057 = {
4859
isa = PBXGroup;
4960
children = (
@@ -63,16 +74,17 @@
6374
695F4865222440B90058E057 /* libSetReplace */ = {
6475
isa = PBXGroup;
6576
children = (
66-
69ED0F4023170D5D0014A48E /* IDTypes.hpp */,
6777
69B2D41122244AFC008778A3 /* Expression.hpp */,
6878
6961A1D523184ABC009461C8 /* Expression.cpp */,
69-
69B2D41422244D1F008778A3 /* Rule.hpp */,
79+
69ED0F4023170D5D0014A48E /* IDTypes.hpp */,
7080
6909EBAA22258611001458D2 /* Match.hpp */,
7181
6909EBA922258611001458D2 /* Match.cpp */,
82+
69B2D41422244D1F008778A3 /* Rule.hpp */,
7283
695F486F222443F20058E057 /* Set.hpp */,
7384
695F486E222443F20058E057 /* Set.cpp */,
74-
69CAF8732257B23B006F9C60 /* SetReplace.cpp */,
7585
69CAF8742257B23B006F9C60 /* SetReplace.hpp */,
86+
69CAF8732257B23B006F9C60 /* SetReplace.cpp */,
87+
691E07792471CED500D2BDD5 /* test */,
7688
);
7789
path = libSetReplace;
7890
sourceTree = "<group>";

libSetReplace/Expression.cpp

Lines changed: 52 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -5,67 +5,67 @@
55
#include <utility>
66

77
namespace SetReplace {
8-
class AtomsIndex::Implementation {
9-
private:
10-
const std::function<AtomsVector(ExpressionID)> getAtomsVector_;
11-
std::unordered_map<Atom, std::unordered_set<ExpressionID>> index_;
8+
class AtomsIndex::Implementation {
9+
private:
10+
const std::function<AtomsVector(ExpressionID)> getAtomsVector_;
11+
std::unordered_map<Atom, std::unordered_set<ExpressionID>> index_;
1212

13-
public:
14-
explicit Implementation(std::function<AtomsVector(ExpressionID)> getAtomsVector)
15-
: getAtomsVector_(std::move(getAtomsVector)) {}
13+
public:
14+
explicit Implementation(std::function<AtomsVector(ExpressionID)> getAtomsVector)
15+
: getAtomsVector_(std::move(getAtomsVector)) {}
1616

17-
void removeExpressions(const std::vector<ExpressionID>& expressionIDs) {
18-
const std::unordered_set<ExpressionID> expressionsToDelete(expressionIDs.begin(), expressionIDs.end());
17+
void removeExpressions(const std::vector<ExpressionID>& expressionIDs) {
18+
const std::unordered_set<ExpressionID> expressionsToDelete(expressionIDs.begin(), expressionIDs.end());
1919

20-
std::unordered_set<Atom> involvedAtoms;
21-
for (const auto& expression : expressionIDs) {
22-
const auto& atomsVector = getAtomsVector_(expression);
23-
involvedAtoms.insert(atomsVector.begin(), atomsVector.end());
24-
}
20+
std::unordered_set<Atom> involvedAtoms;
21+
for (const auto& expression : expressionIDs) {
22+
const auto& atomsVector = getAtomsVector_(expression);
23+
involvedAtoms.insert(atomsVector.begin(), atomsVector.end());
24+
}
2525

26-
for (const auto& atom : involvedAtoms) {
27-
auto& atomExpressionSet = index_[atom];
28-
auto atomExpressionIterator = atomExpressionSet.begin();
29-
const auto atomExpressionSetEnd = atomExpressionSet.cend();
30-
while (atomExpressionIterator != atomExpressionSetEnd) {
31-
if (expressionsToDelete.count(*atomExpressionIterator)) {
32-
atomExpressionIterator = atomExpressionSet.erase(atomExpressionIterator);
33-
} else {
34-
++atomExpressionIterator;
35-
}
36-
}
37-
if (atomExpressionSet.empty()) {
38-
index_.erase(atom);
39-
}
40-
}
26+
for (const auto& atom : involvedAtoms) {
27+
auto& atomExpressionSet = index_[atom];
28+
auto atomExpressionIterator = atomExpressionSet.begin();
29+
const auto atomExpressionSetEnd = atomExpressionSet.cend();
30+
while (atomExpressionIterator != atomExpressionSetEnd) {
31+
if (expressionsToDelete.count(*atomExpressionIterator)) {
32+
atomExpressionIterator = atomExpressionSet.erase(atomExpressionIterator);
33+
} else {
34+
++atomExpressionIterator;
4135
}
36+
}
37+
if (atomExpressionSet.empty()) {
38+
index_.erase(atom);
39+
}
40+
}
41+
}
4242

43-
void addExpressions(const std::vector<ExpressionID>& expressionIDs) {
44-
for (const auto& expressionID : expressionIDs) {
45-
for (const auto& atom : getAtomsVector_(expressionID)) {
46-
index_[atom].insert(expressionID);
47-
}
48-
}
49-
}
43+
void addExpressions(const std::vector<ExpressionID>& expressionIDs) {
44+
for (const auto& expressionID : expressionIDs) {
45+
for (const auto& atom : getAtomsVector_(expressionID)) {
46+
index_[atom].insert(expressionID);
47+
}
48+
}
49+
}
5050

51-
std::unordered_set<ExpressionID> expressionsContainingAtom(const Atom atom) const {
52-
const auto resultIterator = index_.find(atom);
53-
return resultIterator != index_.end() ? resultIterator->second : std::unordered_set<ExpressionID>();
54-
}
55-
};
51+
std::unordered_set<ExpressionID> expressionsContainingAtom(const Atom atom) const {
52+
const auto resultIterator = index_.find(atom);
53+
return resultIterator != index_.end() ? resultIterator->second : std::unordered_set<ExpressionID>();
54+
}
55+
};
5656

57-
AtomsIndex::AtomsIndex(const std::function<AtomsVector(ExpressionID)>& getAtomsVector)
58-
: implementation_(std::make_shared<Implementation>(getAtomsVector)) {}
57+
AtomsIndex::AtomsIndex(const std::function<AtomsVector(ExpressionID)>& getAtomsVector)
58+
: implementation_(std::make_shared<Implementation>(getAtomsVector)) {}
5959

60-
void AtomsIndex::removeExpressions(const std::vector<ExpressionID>& expressionIDs) {
61-
implementation_->removeExpressions(expressionIDs);
62-
}
60+
void AtomsIndex::removeExpressions(const std::vector<ExpressionID>& expressionIDs) {
61+
implementation_->removeExpressions(expressionIDs);
62+
}
6363

64-
void AtomsIndex::addExpressions(const std::vector<ExpressionID>& expressionIDs) {
65-
implementation_->addExpressions(expressionIDs);
66-
}
64+
void AtomsIndex::addExpressions(const std::vector<ExpressionID>& expressionIDs) {
65+
implementation_->addExpressions(expressionIDs);
66+
}
6767

68-
std::unordered_set<ExpressionID> AtomsIndex::expressionsContainingAtom(const Atom atom) const {
69-
return implementation_->expressionsContainingAtom(atom);
70-
}
68+
std::unordered_set<ExpressionID> AtomsIndex::expressionsContainingAtom(const Atom atom) const {
69+
return implementation_->expressionsContainingAtom(atom);
7170
}
71+
} // namespace SetReplace

libSetReplace/Expression.hpp

Lines changed: 55 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,63 +1,63 @@
1-
#ifndef Expression_hpp
2-
#define Expression_hpp
1+
#ifndef LIBSETREPLACE_EXPRESSION_HPP_
2+
#define LIBSETREPLACE_EXPRESSION_HPP_
33

44
#include <functional>
55
#include <memory>
6-
#include <vector>
76
#include <unordered_set>
7+
#include <vector>
88

99
#include "IDTypes.hpp"
1010

1111
namespace SetReplace {
12-
/** @brief List of atoms without references to events, as can be used in, i.e., rule specification.
13-
*/
14-
using AtomsVector = std::vector<Atom>;
15-
16-
/** @brief Expression, as a part of the set, i.e., (hyper)edges in the graph.
17-
*/
18-
struct SetExpression {
19-
/** @brief Ordered list of atoms the expression contains.
20-
*/
21-
AtomsVector atoms;
22-
23-
/** @brief Substitution event that has this expression as part of its output.
24-
*/
25-
EventID creatorEvent;
26-
27-
/** @brief Substitution event that has this expression as part of its input.
28-
*/
29-
EventID destroyerEvent = finalStateEvent;
30-
31-
/** @brief Layer of the causal network this expression belongs to.
32-
*/
33-
Generation generation;
34-
};
35-
36-
/** @brief AtomsIndex keeps references to set expressions accessible by atoms, which is useful for matching.
37-
*/
38-
class AtomsIndex {
39-
public:
40-
/** @brief Creates an empty index.
41-
* @param getAtomsVector datasource function that returns the list of atoms for a requested expression.
42-
*/
43-
explicit AtomsIndex(const std::function<AtomsVector(ExpressionID)>& getAtomsVector);
44-
45-
/** @brief Removes expressions with specified IDs from the index.
46-
*/
47-
void removeExpressions(const std::vector<ExpressionID>& expressionIDs);
48-
49-
/** @brief Adds expressions with specified IDs to the index.
50-
*/
51-
void addExpressions(const std::vector<ExpressionID>& expressionIDs);
52-
53-
/** @brief Returns the list of expressions containing a specified atom.
54-
*/
55-
std::unordered_set<ExpressionID> expressionsContainingAtom(Atom atom) const;
56-
57-
private:
58-
class Implementation;
59-
std::shared_ptr<Implementation> implementation_;
60-
};
61-
}
62-
63-
#endif /* Expression_hpp */
12+
/** @brief List of atoms without references to events, as can be used in, i.e., rule specification.
13+
*/
14+
using AtomsVector = std::vector<Atom>;
15+
16+
/** @brief Expression, as a part of the set, i.e., (hyper)edges in the graph.
17+
*/
18+
struct SetExpression {
19+
/** @brief Ordered list of atoms the expression contains.
20+
*/
21+
AtomsVector atoms;
22+
23+
/** @brief Substitution event that has this expression as part of its output.
24+
*/
25+
EventID creatorEvent;
26+
27+
/** @brief Substitution event that has this expression as part of its input.
28+
*/
29+
EventID destroyerEvent = finalStateEvent;
30+
31+
/** @brief Layer of the causal network this expression belongs to.
32+
*/
33+
Generation generation;
34+
};
35+
36+
/** @brief AtomsIndex keeps references to set expressions accessible by atoms, which is useful for matching.
37+
*/
38+
class AtomsIndex {
39+
public:
40+
/** @brief Creates an empty index.
41+
* @param getAtomsVector datasource function that returns the list of atoms for a requested expression.
42+
*/
43+
explicit AtomsIndex(const std::function<AtomsVector(ExpressionID)>& getAtomsVector);
44+
45+
/** @brief Removes expressions with specified IDs from the index.
46+
*/
47+
void removeExpressions(const std::vector<ExpressionID>& expressionIDs);
48+
49+
/** @brief Adds expressions with specified IDs to the index.
50+
*/
51+
void addExpressions(const std::vector<ExpressionID>& expressionIDs);
52+
53+
/** @brief Returns the list of expressions containing a specified atom.
54+
*/
55+
std::unordered_set<ExpressionID> expressionsContainingAtom(Atom atom) const;
56+
57+
private:
58+
class Implementation;
59+
std::shared_ptr<Implementation> implementation_;
60+
};
61+
} // namespace SetReplace
62+
63+
#endif // LIBSETREPLACE_EXPRESSION_HPP_

0 commit comments

Comments
 (0)