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

Made it build on EPICS 3.14.12.6, all unit tests pass. #3

Closed
wants to merge 4 commits into from

Conversation

klemenv
Copy link
Contributor

@klemenv klemenv commented Apr 14, 2020

No description provided.

@mdavidsaver
Copy link
Member

Ha, I didn't know about std::stoul and friends.

Could you also add a travis-ci job for 3.14? This should be as simple as copying the 3.15 job and changing to "BASE=3.14".

https://github.com/mdavidsaver/pvxs/blob/6bb0a364bed580064148031cd76051e74f1b385a/.travis.yml#L71-L78

@mdavidsaver
Copy link
Member

I didn't know about std::stoul and friends.

FYI. like strtoul() and friends, the std::sto*() functions ignore extra trailing characters. So eg. given "0 .5", std::stod() happily stops at the space and return zero. The epicsParse*() wrappers have an extra check for this, and return S_stdlib_extraneous. That said, using std::sto*, with appropriate error handling, seems like the way to go.

@klemenv
Copy link
Contributor Author

klemenv commented Apr 15, 2020

I addressed std::sto* properly but haven't pushed it yet. I was trying out travis integration and it pointed out a linker problem that I've been fighting since then. Basically tools/info.cpp doesn't link in my epicsParse* functions and I can't figure out what's wrong with it. It's both C++ code, but I did try with extern "C" to no avail. I figured it wouldn't hurt to add epicsShareFunc too, although doesn't have any change on Linux/g++. nm finds the functions and obviously they are part of .so since they're used by other functions, so I'm clueless. Any tips?

@mdavidsaver
Copy link
Member

I figured it wouldn't hurt to add epicsShareFunc too

You're on the right track. However, try PVXS_API instead of epicsShareFunc. This has the same effect, but doesn't depend on #include ordering.

@klemenv
Copy link
Contributor Author

klemenv commented Apr 16, 2020

With PVXS_API it seems to be just working now. Travis jobs were failing in my local repo too, but succeeded when I restarted them. Not related to this patch.

@mdavidsaver mdavidsaver mentioned this pull request Apr 17, 2020
@mdavidsaver
Copy link
Member

Can you have a look at #4? The main difference is to remove use of epicsParse* entirely. Now that I know about them, the std::sto*() family of functions seem like a better fit. Also, as I thought about it. I don't like the idea of libpvxs exporting symbols which normally come from libCom.

src/util.cpp Outdated
try {
size_t idx;
epicsInt8 v = (std::stoi(s, &idx, base) & 0xff);
if (idx == std::string(s).length()) {
Copy link
Member

Choose a reason for hiding this comment

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

Creating temporary std::strings like this is not so efficient. strlen() is entirely appropriate. Or if you want to be super efficient, test if s[idx]=='\0'. As I read it idx will always point to a valid character, which includes the nil.

epicsEnvUnset("EPICS_PVA_BROADCAST_PORT");
//epicsEnvUnset("EPICS_PVA_ADDR_LIST");
//epicsEnvUnset("EPICS_PVA_AUTO_ADDR_LIST");
//epicsEnvUnset("EPICS_PVA_BROADCAST_PORT");
Copy link
Member

Choose a reason for hiding this comment

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

I'd forgotten that this was such a recent addition.

@mdavidsaver
Copy link
Member

Superseded by #4

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.

2 participants