Added backward compatible API for clgetSVal(), application level include file, and more unit tests#36
Conversation
…e API clgetSVal() work.
clgetBaseCode.h, clgetValp.cc, clparseVal.cc: Remove debugging cout
setAutoDefaults.h: Setting S->Val to S->Defaults if S->NVals == 0.
ParafeedTest.h: Added more tests in FactoryCanonical test.
_parafeed_interactive.cpp: Using FactoryCanonical() in a new test to
test the case where only factory defaults
are used.
ParafeedTest.h: Added tests for the above interface.
ParseCmdLine.cc: Enabled loadDefaults() in startShell(). _interactive.cpp: Added the test for auto loding the .def file, if found. Whitespace cleanup throughout.
Reverted to complining clgetValp.cc. Keeping it header-only needs carefully organized code (especially templates). Using parafeed.h in testing code. Added copyright header where it was missing.
clinteract.h, parafeed.h: Moved clgetValp.cc from clinteract.h to parafeed.h
Added include guard in parafeed.h
| string valp; | ||
| int r; | ||
| if ((r = clgetSValp(std::string(Name), valp, *n,smap,dbg))!= CL_FAIL) | ||
| strncpy(val,valp.c_str(),valp.size()+1); |
There was a problem hiding this comment.
This may cause overflow when the val buffer is smaller than the valp size since we cannot guarantee users set the the size of val to be big enough. Should be fine for now since the current users (us) know what they are doing and this should happen rarely.
There was a problem hiding this comment.
This is a fundamental problem with the C API (clgetSValp() interface). There is no way to know the size of allocated memory that valp points to. And therefore no way to guard against overshooting in strncpy. Since this is a library call, strdup() will also not be useful. Replacing valp with a new pointer may lead to memory leak in the client code. Such limitations are also the primary reasons to move the library code to use C++ containers which when used properly can ensure memory safety (and one can always produce a C-API for C++ code).
Recall that C-API has only backward-compatibility value. And of course, if parafeed is every used in a pure-C code that can't be build with C++ compiler. It is quite straight forward to write C code using C++ containers these days, but of course that needs C++-11 or later (C++98 won't do it).
| HANDLE_EXCEPTIONS( | ||
| if (S != NULL) | ||
| { | ||
| // Transfer S-DefaultVal to S->Val if S->NVals == 0. Is this |
There was a problem hiding this comment.
Not sure if the downstream logic does things differently if a parameter is unset. However, with this change we cannot distinction between “unset” vs “defaulted” states. One example I can think of is
before the code change,
scale=[]
makes multiscale internally determines a list of scales. However, with the change the scale=[] now becomes scale=default scales which alters the behavior.
There was a problem hiding this comment.
The logic here is that "unset" values will always be determined by the "default" values. The latter are set by the programmer in the application layer (the initialized values of the val in clgetValp('...', val, i)). So, for the above example, default value for scale should be blank in the client code.
The alternative logic could be that the application starts with the programmer-defined default value, and the user can set it (as now), or unset it. In the latter case, there will be no way to get back to the programmer-defined defaults.
This change was actually made since the return value of the clgetValp() functions in the first pass is zero, even when the value displayed has the programmer-defined values (which could be non-zero list). So I felt that this is inconsistent and therefore a bug. This is now tested in ParafeedTests.h:FactoryCanonicalTest().
For the LibRA example you give above -- is that tested via LibRA unit test suite? I have checked that those all pass.
There was a problem hiding this comment.
The scenario I mentioned about scale is not tested in the libra unit tests, but like you said as long as the users/programmers carefully set the default values, this scenario should not happen.
Thank you for replying both of my comments. The PR looks good and can be merged now.
3 participants
[dbg]getSVal()API calls for backward compatibility. But not[dbg]clgetNSVal(..., char **,...)calls. These seem more cumbersome to use and will be added if and when necessary.parafeed.h. Including this is the applications is now necessary (and recommended way interfacing with the library).<AppName>.deffile. All errors in this file are now printed with line number information before the application exits.