Revamp configuration implementation mechanism. #127
Conversation
igorpeshansky
force-pushed
the
igorp-config-revamp
branch
from
a7fdd07 to
8aae33a
Compare
|
Rebased this against the latest |
src/Makefile
Outdated
| UNAME_S=$(shell uname -s) | ||
| ifeq ($(UNAME_S),Darwin) | ||
| CPPFLAGS+= -I/usr/local/include -I/usr/local/opt/openssl/include | ||
| CPPFLAGS+= -UNOOPT -DNOOPT='[[clang::optnone]]' -I/usr/local/include -I/usr/local/opt/openssl/include |
There was a problem hiding this comment.
Add a comment explaining why we need these options.
There was a problem hiding this comment.
No longer relevant, so removed.
src/configuration.h
Outdated
|
|
||
| // Shared configuration. | ||
| const std::string& ProjectId() const { | ||
| NOOPT const std::string& ProjectId() const { |
There was a problem hiding this comment.
Also comment on the NOOPTs in this file.
There was a problem hiding this comment.
Uh, apologies for jumping in here, but [[clang:optnone]] is not guaranteed to be honoured even by clang. If you want to force the function to never be inlined, use a different attribute. In this case, __attribute__((noinline)) is supported by both clang and gcc.
There was a problem hiding this comment.
There's definitely something fishy on MacOS with respect to optimizing those functions. It used to crash for me with __attribute__((noinline)), but not with [[clang::optnone]], but no longer seems to. I'll just remove it for now, and we can investigate if the crashes reoccur.
igorpeshansky
left a comment
igorpeshansky
left a comment
There was a problem hiding this comment.
Thanks. PTAL.
src/configuration.h
Outdated
|
|
||
| // Shared configuration. | ||
| const std::string& ProjectId() const { | ||
| NOOPT const std::string& ProjectId() const { |
There was a problem hiding this comment.
There's definitely something fishy on MacOS with respect to optimizing those functions. It used to crash for me with __attribute__((noinline)), but not with [[clang::optnone]], but no longer seems to. I'll just remove it for now, and we can investigate if the crashes reoccur.
src/Makefile
Outdated
| UNAME_S=$(shell uname -s) | ||
| ifeq ($(UNAME_S),Darwin) | ||
| CPPFLAGS+= -I/usr/local/include -I/usr/local/opt/openssl/include | ||
| CPPFLAGS+= -UNOOPT -DNOOPT='[[clang::optnone]]' -I/usr/local/include -I/usr/local/opt/openssl/include |
There was a problem hiding this comment.
No longer relevant, so removed.
igorpeshansky
force-pushed
the
igorp-config-revamp
branch
from
ff02bae to
ec13632
Compare
|
Rebased off |
igorpeshansky
changed the title
4 participants
This will probably not get merged in its current state, but wanted to start a discussion on a better configuration mechanism implementation.