NilFoundation / zkllvm-assigner

MIT License
12 stars 11 forks source link

Refine include guards in all header files #213

Closed aleasims closed 6 months ago

aleasims commented 6 months ago

Current state

We have a really messy include guards at the moment actually. E.g. assigner.hpp has:

#define CRYPTO3_BLUEPRINT_COMPONENT_INSTRUCTION_PARSER_HPP

which doesn't make any sense at all now. Some of the files have something mixed like:

#define CRYPTO3_ASSIGNER_NIL_BLUEPRINT_ASSERTS_HPP

which is also a bit confusing. So it's really hard to say, which style we use in this project.

Using CRYPTO3_ASSIGNER doesn't makes any sense for me: there is nothing related to crypto3 here. assigner is not a part of crypto3.

Suggestion

There are some style guides like this one from Google, which proposes <PROJECT>_<PATH>_<FILE>_H_. So it may be:

#define ZKLLVM_ASSIGNER_INCLUDE_NIL_BLUEPRINT_ASSIGNER_HPP_

We probably may expect, that prefix ZKLLVM_ASSIGNER_ is good enough for the uniqueness on user side. So we probably can omit include/nil/blueprint part of the path. At the end we will have this:

#define ZKLLVM_ASSIGNER_ASSIGNER_HPP_

So out include guards may look like <PROJECT>_<PATH_FROM_HEADERS_DIR>_<FILE>_HPP_.

Examples:

// zkllvm-assigner/include/nil/blueprint/assigner.hpp
#define ZKLLVM_ASSIGNER_ASSIGNER_HPP_

// zkllvm-assigner/include/nil/blueprint/curves/addition.hpp
#define ZKLLVM_ASSIGNER_CURVES_ADDITION_HPP_

// zkllvm-assigner/include/nil/blueprint/mem/segment.hpp
#define ZKLLVM_ASSIGNER_MEM_SEGMENT_HPP_
aleasims commented 6 months ago

@akokoshn @makxenov expecting you to comment

makxenov commented 6 months ago

I vote for the original Google recommendation, because removing some specific parts may be confusing. For example, someone takes ZKLLVM_ASSIGNER_ASSIGNER_HPP_ as a example and adds ZKLLVM_ASSIGNER_SEGMENT_HPP_, omitting the intermediate mem directory. I don't see a problem with longer include guards.

akokoshn commented 6 months ago

Agree: <PROJECT>_<PATH>_<FILE>_H_ looks like best solution