sahrk
commented
3 years ago @r-barnes I pushed a fix to master. Please let me know if it does the trick.
Closed r-barnes closed 2 years ago
sahrk
commented
3 years ago @r-barnes I pushed a fix to master. Please let me know if it does the trick.
r-barnes
commented
3 years ago This seems to work, thanks!
edzer
commented
3 years ago With R CMD check --use-valgrind, there are no more memory leaks but still a few problems:
(Ingore the ones about the vignettes, I build using R CMD build --no-build-vignettes)
Branch update_from_upstream gives
r-barnes
commented
3 years ago @edzer : Thanks for circling back on this. The update_from_upstream branch is merged, so I'm working off of master now.
I see four potential issues above.
(1) Call sites to various standard library functions output/system functions. I'll talk with @sahrk to see if we can come up with a better solution to these. Perhaps you could suggest a good C++ logging library?
(2) Something about the license. I've used usethis::use_mit_license() to regenerate the license. I hope this fixes things.
(3) @sahrk might know what's going on here.
(4) A leak and heap summary. I don't see anything that concerns me in this summary. Did you see anything there you wanted to comment on?
sahrk
commented
2 years ago @edzer I'm going to reply to @r-barnes enumeration of problems. Please let me know if something important was missed.
I see four potential issues above.
(1) Call sites to various standard library functions output/system functions. I'll talk with @sahrk to see if we can come up with a better solution to these. Perhaps you could suggest a good C++ logging library?
It doesn't look like there are too many spots where output is occurring (most of it goes through the DgBase::report() method). So for now we could use a build switch to conditionally remove the output statements. Of course that means you won't get any of the error messages.
(2) Something about the license. I've used
usethis::use_mit_license()to regenerate the license. I hope this fixes things.
The license should be the GNU Affero General Public License v3.0, not MIT.
(3) @sahrk might know what's going on here.
Not sure what part you're referring to here. I will suppress that "invalid size setting" warning, which is not actually a problem in most cases (I need to do a more robust check for actual overflow).
r-barnes
commented
2 years ago @sahrk :
std::cout, std::cerr, and printf statements.sahrk
commented
2 years ago @r-barnes:
- A build switch would probably work fine. However, if everything goes through report, then I can set report up to redirect to R's warning/error system. The issue is any
std::cout,std::cerr, andprintfstatements.
I put conditional compile code in the report method in DgBase.cpp, using a macro DGGRIDR. If you compile with DGGRIDR defined there should be no output from report. You can insert whatever output code is appropriate for dggridR in the #ifdef.
There are other uses of cout, etc. in the library, but these will never be executed unless you call them. Is that acceptable to CRAN? For example, debugging statements that DGGRID would only execute if the verbosity parameter is high enough. It would take some time (and add to the unreadability of the code) to conditionally compile all of these spots.
- The license on the dggridR part of the code is (or can be) MIT. I'll look through the licensing stuff to see what I can do about sorting this out.
The dglib/DGGRID code is AGPL. I assumed that meant that dggridR had to be too. But now I realize that I don't really know if that's true...
- I'm not sure what I'm referring to either, but I'm betting it's the "invalid size setting" warning. It'd be nice if we either had a clean error or no message.
I have commented out all the "possible overflow" warning.
r-barnes
commented
2 years ago @sahrk : Thanks for adding the compilation flag.
I think CRAN will flag any references it sees to things like stdout as an issue; it only wants to have output passed through Rcpp::cout. If it's possible to pass all the library's output through some sort of logger that's probably the best way to handle this.
One thought would be to use a header-only logger library like spdlog as a way of (hopefully) simplifying the logging code while applying it to these other use points.
If DGGRID is AGPL, then I will switch dggridR to that as well: it's not much use without DGGRID and it's best not to confuse folks.
r-barnes
commented
2 years ago @sahrk : I've updated the dggridR license file to AGPL and you've followed up on the logging elsewhere, so I'm closing this issue.
In the file
DGGRID/src/lib/dglib/include/dglib/DgDiscRFS.hI'm seeing the following variables of the classDgResAddas potentially uninitialized when used:Are there reasonable default values for these variables that will preserve the intended behaviour of the program? (For completeness, it'd be good to give good defaults to the other classes here as well.)
The full warning is shown below: