various object placement and constness improvements#7138
various object placement and constness improvements#7138MichaelChirico merged 21 commits intomasterfrom
Conversation
|
No obvious timing issues in HEAD=DataCodeIntegration Generated via commit 398f117 Download link for the artifact containing the test results: ↓ atime-results.zip
|
|
Please check the CI failure |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7138 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 81 81
Lines 15185 15197 +12
=======================================
+ Hits 14999 15011 +12
Misses 186 186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Basically looks good, could you please add some commentary for my edification & posterity? |
|
This change is still pending new tests as requested |
|
Am I expected to write these tests? I'm not proficient in R. |
|
No time like the present to improve your proficiency. |
|
I added @aitap 's test to the testing suite. It's giving a warning, I'm not sure why. |
|
Thank you! |
| // \r is 13, \n is 10, and \0 is 0. The second part is optimized based on the | ||
| // fact that the characters in the ASCII range 0..13 are very rare, so a | ||
| // single check `ch<=13` is almost equivalent to checking whether `ch` is one | ||
| // of \r, \n, \0. We cast to unsigned first because `char` type is signed by | ||
| // default, and therefore characters in the range 0x80-0xFF are negative. | ||
| // We use eol() because that looks at eol_one_r inside it w.r.t. \r | ||
| // \0 (maybe more than one) before eof are part of field and do not end it; eol() returns false for \0 but the ch==eof will return true for the \0 at eof. | ||
| return *ch == sep || ((uint8_t)*ch <= 13 && (ch == eof || eol(&ch))); | ||
| return *ch == sep || (*ch <= 13 && (ch == eof || eol(&ch))); |
There was a problem hiding this comment.
Sorry for missing this the first time, but the comment is now wrong. eol() is now called for
| SEXP tt, ss = R_NilValue; | ||
| setAttrib(DT, R_NamesSymbol, tt = PROTECT(allocVector(STRSXP, ncol - ndrop))); nprotect++; | ||
| SEXP tt = PROTECT(allocVector(STRSXP, ncol - ndrop)); | ||
| setAttrib(DT, R_NamesSymbol, tt); nprotect++; |
There was a problem hiding this comment.
Stylistic: better keep the nprotect++ on the same line as the PROTECT() it's marking. setAttrib(), on the other hand, does not touch the protection stack.
3 participants
I was told previously to work on bigger PRs.
This one focuses primarily on object declarations, placements, and constness to further clarify intent earlier in the code.
After this I want to redo the naming scheme for some of the parsing functions and redo the parameters to return objects instead of using return pointers and structs.
I also want to arrange all the benchmarking data into a single struct for clarity. Often the timestamps are held in objects with unclear names.