Feature/rm cincludes #37

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
@sjaeckel
Member

sjaeckel commented Feb 18, 2014

We had several suggestions how to remove the included c-files, I propose this solution...

@sjaeckel sjaeckel referenced this pull request Feb 18, 2014

Closed

release needed - soon #36

@sjaeckel sjaeckel added this to the v1.18.0 milestone Feb 25, 2014

@karel-m

This comment has been minimized.

Show comment
Hide comment
@karel-m

karel-m Feb 25, 2014

Member

Makefiles need to update the following part:

#ciphers come in two flavours... enc+dec and enc
src/ciphers/aes/aes_enc.o: src/ciphers/aes/aes.c src/ciphers/aes/aes_tab.c src/ciphers/aes/aes_tab.h
    $(CC) $(CFLAGS) -DENCRYPT_ONLY -c src/ciphers/aes/aes.c -o src/ciphers/aes/aes_enc.o

#These are the rules to make certain object files.
src/ciphers/aes/aes_tab.o: src/ciphers/aes/aes_tab.c src/ciphers/aes/aes_tab.h
src/ciphers/twofish/twofish_tab.o: src/ciphers/twofish/twofish_tab.c src/ciphers/twofish/twofish_tab.h
src/ciphers/safer/safer_tab.o: src/ciphers/safer/safer_tab.c src/ciphers/safer/safer_tab.h
src/hashes/whirl/whirltab.o: src/hashes/whirl/whirltab.c src/hashes/whirl/whirltab.h
src/prngs/sober128tab.o: src/prngs/sober128tab.c src/prngs/sober128tab.h
src/pk/dh/dh_static.o: src/pk/dh/dh_static.c src/pk/dh/dh_static.h
Member

karel-m commented Feb 25, 2014

Makefiles need to update the following part:

#ciphers come in two flavours... enc+dec and enc
src/ciphers/aes/aes_enc.o: src/ciphers/aes/aes.c src/ciphers/aes/aes_tab.c src/ciphers/aes/aes_tab.h
    $(CC) $(CFLAGS) -DENCRYPT_ONLY -c src/ciphers/aes/aes.c -o src/ciphers/aes/aes_enc.o

#These are the rules to make certain object files.
src/ciphers/aes/aes_tab.o: src/ciphers/aes/aes_tab.c src/ciphers/aes/aes_tab.h
src/ciphers/twofish/twofish_tab.o: src/ciphers/twofish/twofish_tab.c src/ciphers/twofish/twofish_tab.h
src/ciphers/safer/safer_tab.o: src/ciphers/safer/safer_tab.c src/ciphers/safer/safer_tab.h
src/hashes/whirl/whirltab.o: src/hashes/whirl/whirltab.c src/hashes/whirl/whirltab.h
src/prngs/sober128tab.o: src/prngs/sober128tab.c src/prngs/sober128tab.h
src/pk/dh/dh_static.o: src/pk/dh/dh_static.c src/pk/dh/dh_static.h
@karel-m

This comment has been minimized.

Show comment
Hide comment
@karel-m

karel-m Feb 25, 2014

Member

I'am not sure if it is necessary to use:
#define __DECL_<name>_TAB_H__ extern

As all the tables are only for internal use and we are not about to expose them from outside the library I guess that it can be always extern

Or perhaps having just one globally defined:
#define __DECL_INTERNAL_TAB_H__ extern
instead on many __DECL_<name>_TAB_H__

But of course it is only cosmetics

Member

karel-m commented Feb 25, 2014

I'am not sure if it is necessary to use:
#define __DECL_<name>_TAB_H__ extern

As all the tables are only for internal use and we are not about to expose them from outside the library I guess that it can be always extern

Or perhaps having just one globally defined:
#define __DECL_INTERNAL_TAB_H__ extern
instead on many __DECL_<name>_TAB_H__

But of course it is only cosmetics

@sjaeckel

This comment has been minimized.

Show comment
Hide comment
@sjaeckel

sjaeckel Feb 25, 2014

Member

mh, regarding #37 (comment) wouldn't it be enought to add the .h files to the HEADERS part of the makefiles?

Member

sjaeckel commented Feb 25, 2014

mh, regarding #37 (comment) wouldn't it be enought to add the .h files to the HEADERS part of the makefiles?

@karel-m

This comment has been minimized.

Show comment
Hide comment
@karel-m

karel-m Feb 25, 2014

Member

Putting ..._tab.h files into HEADERS is enough.

However $(HEADERS) is also used for install target and I guess we do not want to install ..._tab.h files

Member

karel-m commented Feb 25, 2014

Putting ..._tab.h files into HEADERS is enough.

However $(HEADERS) is also used for install target and I guess we do not want to install ..._tab.h files

src/ciphers/aes/aes_tab.c
+#define __DECL_AES_TAB_H__
+#include "aes_tab.h"
+
+const ulong32 TE0[256] = {

This comment has been minimized.

@rofl0r

rofl0r Feb 26, 2014

why was static removed ? is the table used from several TUs ? if so it may make sense to put it into a separate compilation unit. if not, this only adds bloat (object size).

@rofl0r

rofl0r Feb 26, 2014

why was static removed ? is the table used from several TUs ? if so it may make sense to put it into a separate compilation unit. if not, this only adds bloat (object size).

@rofl0r

This comment has been minimized.

Show comment
Hide comment
@rofl0r

rofl0r Feb 26, 2014

since you asked for comments:
i'm personally not too happy with this. why is the change needed ? the only advantage i can see is for a simple buildsystem where you can basically just compile *.c and then link it together. however the current buildsystem seems far from simple anyway...
so from my POV, this is a pretty invasive change for no real gain (that i can see).

rofl0r commented Feb 26, 2014

since you asked for comments:
i'm personally not too happy with this. why is the change needed ? the only advantage i can see is for a simple buildsystem where you can basically just compile *.c and then link it together. however the current buildsystem seems far from simple anyway...
so from my POV, this is a pretty invasive change for no real gain (that i can see).

@karel-m

This comment has been minimized.

Show comment
Hide comment
@karel-m

karel-m Feb 26, 2014

Member

@rofl0r for example src/ciphers/aes/aes_tab.c is used in src/ciphers/aes/aes.c + src/mac/pelican/pelican.c (I have not checked others)

As for the building "by just compiling *.c" in fact it works quite fine (after eliminating .c includes) - I use exactly this approach in my perl bindings

Member

karel-m commented Feb 26, 2014

@rofl0r for example src/ciphers/aes/aes_tab.c is used in src/ciphers/aes/aes.c + src/mac/pelican/pelican.c (I have not checked others)

As for the building "by just compiling *.c" in fact it works quite fine (after eliminating .c includes) - I use exactly this approach in my perl bindings

@sjaeckel

This comment has been minimized.

Show comment
Hide comment
@sjaeckel

sjaeckel Mar 4, 2014

Member

The more I looked at the old patch the less I liked it...
I decided to stay with the old approach for the tabs, but harden it a bit by protecting them, so you could theoretically also "just compile *.c" I think.
for sha2 and dh I decided to remove the inclusion of the c files.

Member

sjaeckel commented Mar 4, 2014

The more I looked at the old patch the less I liked it...
I decided to stay with the old approach for the tabs, but harden it a bit by protecting them, so you could theoretically also "just compile *.c" I think.
for sha2 and dh I decided to remove the inclusion of the c files.

@sjaeckel sjaeckel closed this Apr 3, 2014

@sjaeckel sjaeckel deleted the feature/rmCincludes branch Apr 3, 2014

@sjaeckel sjaeckel modified the milestone: v2.0.0 Feb 21, 2017

@karel-m karel-m referenced this pull request Apr 30, 2017

Merged

more linting #202

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