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

Some code cleanup #5168

Open
chriseth opened this Issue Oct 8, 2018 · 5 comments

Comments

3 participants
@chriseth
Contributor

chriseth commented Oct 8, 2018

From the zeppelin audit:

Some suggestions. This is not mandatory, only perform the changes where it makes sense.

  • use emplace_back instead of push_back #5443
  • Use = default to define a trivial default constructor.
  • Use ClassName x{constrArg} instead of ClassName x = ClassName(constrArg)

  • replace all NULL by nullptr (not in imported code, but in Token.h) #5492`
  • use empty() instead of size() == 0
  • Use the same name for all parameters in the declaration and implementation.
  • Use cctype instead of deprecated C++ header ctype.h.
  • Use cstdio instead of deprecated C++ header stdio.h.
  • When a function is declared with an override remove the redundant virtual.
  • Avoid using static inside anonymous namespaces, because namespace limits the visibility of definitions to a single translation unit.
  • Use finds with a single character string literals when possible (for example, ‘\n’ instead of “\n”).
  • Use const references to loop variables that are copied but only used as const references
  • If a variable is copy-constructed (auto) from a const reference, use const &
  • Remove std::move on const and trivially-copyable types.

@chriseth chriseth added this to Inbox in Zeppelin Audit via automation Oct 8, 2018

@axic

This comment has been minimized.

Member

axic commented Oct 9, 2018

replace all NULL by nullptr (not in imported code, but in Token.h)

Actually the NULL is only used in external C APIs (when using compileStandard) and in Token.h. I am not convinced these need to be changed.

Use finds with a single character string literals when possible (for example, ‘\n’ instead of “\n”).

There seems to be a single instance of this.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Oct 9, 2018

I think we could change Token.h to be more C++-like, perhaps even getting rid of the macros and rather doing it in the way it is done in the RuleList https://github.com/ethereum/solidity/blob/develop/libevmasm/RuleList.h#L61

@chriseth

This comment has been minimized.

Contributor

chriseth commented Oct 9, 2018

Removing the macro in Token.h would help IDEs properly recognizing the token identifiers.

@axic axic referenced this issue Oct 9, 2018

Merged

Some C++ cleanup #5180

@axic axic moved this from Inbox to Ready to be worked on in Zeppelin Audit Oct 9, 2018

@chriseth chriseth added this to Inbox in 0.5.1 via automation Nov 8, 2018

@axic

This comment has been minimized.

Member

axic commented Nov 15, 2018

Avoid using static inside anonymous namespaces, because namespace limits the visibility of definitions to a single translation unit.

The single example I could find is in libsolc for static string s_outputBuffer. Outside of that the CLI code is using it, but it isn't using anonymous namespaces.

@Mordax

This comment has been minimized.

Contributor

Mordax commented Nov 15, 2018

Interested in tackling some of these, if that's cool.

@bit-shift bit-shift removed this from Inbox in 0.5.1 Dec 5, 2018

@bit-shift bit-shift added this to Inbox in 0.5.2 via automation Dec 5, 2018

@chriseth chriseth moved this from Inbox to Ready to be worked on in 0.5.2 Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment