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

Put symhacks in crypto.h; dont edit generated file. [1.1.1] #9281

Closed

Conversation

mspncp
Copy link
Contributor

@mspncp mspncp commented Jul 1, 2019

This is a delayed backport of #8397 to 1.1.1 (see #8397 (comment)).

richsalz and others added 2 commits July 1, 2019 09:47
This does no harm, and ensures that the inclusion isn't mistakenly
removed in the generated *err.h where it's actually needed.

Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from openssl#8397)

(cherry picked from commit b53c4fe)
@@ -455,6 +453,10 @@ sub help
#ifndef HEADER_${lib}ERR_H
# define HEADER_${lib}ERR_H

# ifndef HEADER_SYMHACKS_H
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still wonder why this guard is necessary...

Copy link
Contributor Author

@mspncp mspncp Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an optimization: these 'external' include guards have the advantage that the compiler (resp. preprocessor) does not need to open and read the entire file over and over again when it's included repeatedly. In the modern days where computers have plenty of RAM and efficient file caching, this optimization might not be so important anymore, but still it might speed up compilation.

Unfortunately, there is still no standardized way to tell the compiler that an include file should be included only once, although most of the modern compilers understand #pragma once, which is most often used in conjunction with normal include guards for portability reasons.

Anyway, this question comes too late for this pull request, since it's only a backport of an already merged pr. If you want the external include guards removed, it would have to be done in a separate pull request.

Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

levitte pushed a commit that referenced this pull request Jul 2, 2019
This does no harm, and ensures that the inclusion isn't mistakenly
removed in the generated *err.h where it's actually needed.

Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>

(cherry picked from commit b53c4fe)

Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #9281)
levitte pushed a commit that referenced this pull request Jul 2, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
(Merged from #9281)
@mspncp
Copy link
Contributor Author

mspncp commented Jul 2, 2019

Merge, thank you!

3003d2d Add regenerated header files
dfaaf47 util/mkerr.pl: Add an inclusion of symhacks.h in all error files

@mspncp mspncp closed this Jul 2, 2019
@mspncp mspncp deleted the pr-cherry-pick-symhacks-111 branch July 2, 2019 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants