Skip to content
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

Clang tidy #362

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Clang tidy #362

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 18, 2018

Small cleanup problems found by clang-tidy. ;)

@ataulien
Copy link
Collaborator

About time this was being done. Thanks!

I checked all replacements and found one instance of where a comment said that iteratiors should not be used there (see in-line comment). Otherwise they should be all good.

@ghost
Copy link
Author

ghost commented Aug 18, 2018

Should be ok now. ;)

@mdrost
Copy link
Contributor

mdrost commented Sep 8, 2018

Removing empty destructors from cpp files and using default in h files could increase executable size because of inlining and that could cause performance drop because of instruction cache miss.

@ghost
Copy link
Author

ghost commented Sep 8, 2018

Because struct A {}; should give almost the same result as struct A{ ~A() = default;}; it would be weird if "default" approach (without defining dtor) gives nasty results.

Furthermore inlining is just clue for compiler, and if compiler generates nasty code, I would say it's a bug.

@frabert
Copy link
Contributor

frabert commented Sep 8, 2018

I'd tend to agree with @ShFil119, there is no guarantee that the compiler is going to inline anything here; in fact, it may well decide that if the ctor/dtor is used often around the program it is not a good fit for inline. On the contrary, it might improve performance in the case where a ctor/dtor is used sparingly (once or twice) around the codebase and it could not have been inlined before because the source was not available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants