white space improvements #8

apmccartney · 2017-11-23T01:45:32Z

Resolves two outstanding issues:

support for reading Windows-style crlf line endings.
support for reading right-justified real number and integer fields.

With respect to the latter, this pull request actually supports arbitrary leading and trailing white space padding. In addition, a few minor performance tweaks were made.

leveraging branching hints where gnu compiler intrinsics are available.
reformulating exponent-less real number parsing in terms of multiplication rather than multiplication.

coveralls · 2017-11-23T02:03:54Z

Coverage increased (+2.1%) to 95.833% when pulling 1e47ba8 on feature/whitespace into f1f2fdf on master.

whaeck · 2017-11-27T17:21:14Z

src/disco/FixedWidthField/Real/Pow.hpp

-    ( power == 1 ) ? real :
+    ( power < 0 )          ? 1.0 / pow( real, -power ) :
+    ( power == 0 )         ? 1.0 :
+    ( power == 1 )         ? real :
    ( ( power % 2 ) == 0 ) ? pow( real * real , power / 2 ) :
                             real * pow( real, power - 1 );


For my personal education: does this type of optimisation for even power values (which I would not have thought of to be honest) have an important impact?

Bear in mind, this is a constexpr function used to populate a few constexpr template variables at compile time. This is definitely not optimal in terms of runtime performance. This sort of recursion would perform terribly.

The even power case here is to limit the recursion depth. This was necessary to compile on earlier versions of Clang, but improves compile time across GCC and Clang even now.

Ah, I did not notice the constexpr when I looked over it :-)

whaeck · 2017-11-27T17:32:44Z

src/disco/FixedWidthField.hpp

  constexpr static bool
-  isSpace( const char c ){ return c == ' ' || c == '\t'; }
+  isSpace( const char c ){ return c == ' ' or c == '\t'; }


There actually is a function from the standard library that does this: std::isblank from the cctype header (space and tab are the only characters defined as blanks). std::isspace includes the space, tab and \n, \r, etc. Unfortunately, there is no corresponding function only for \n, \r, etc.

the minor distinction with respect to the std function is constexpr friendliness, although I don't know that I'm actually taking advantage of that anywhere.

whaeck · 2017-11-27T17:39:09Z

src/disco/FixedWidthField/Integer/src/parseInteger.hpp

+  if ( ( *it == '-' ) or ( *it == '+' ) ){
+    if ( unlikely( ++position == w ) ){
+      throw std::runtime_error("only sign found in integer field");
+    }
    sign -= ( *it == '-' ) * 2;


For my personal education: why not simply write

if ( *it == '-' ) { sign = -1; }

or is there some optimisation ''magic'' I am missing?

There is but in this case, it mostly personal preference.

if requires a branch. Logical and arithmetic operations do not.

whaeck · 2017-11-27T17:50:50Z

src/disco/FixedWidthField/Integer/src/parseInteger.hpp

    ++it;
+    ++position;
  }
  return x;
 }


For my personal education: the standard library defines function like std::atoi and std::stoi (and equivalents for long and long long). Why did you not use one of these functions?

An example may help. Consider the string

11114444\0

where the string contains two integers 1111 and 4444. With either atoi or stoi, each will return a single value: 11114444. In other words, they no way of extracting a value from a substring, baring creating a temporary variable in client code.

Even if that were a reasonable cost, by using these functions, the parser would less robust. Consider the following strings:
0, 00ABC, ABC, 01ABC

Ideally, the last three would be rejected. However, with atoi, the first three strings would return 0 and the forth would return 1. With stoi, all but the third string would be accepted.

I see. This way, you can parse the original string without having to cut it up as long as you know the number of characters you have to read. No copies = better.

whaeck · 2017-11-27T18:13:18Z

src/disco/FixedWidthField/Real/src/parseBase.hpp

@@ -1,11 +1,12 @@
 template< typename Iterator >
 static uint64_t
 parseBase( Iterator& it, uint16_t& position, bool& success ){
-  uint64_t base = ( success = isdigit( *it ) ) ? (*it - 48) : 0 ;
-  if ( success ){ 
+  uint64_t base = ( success = isdigit( *it ) ) ? (*it - 48) : 0;


I assume this is std::isdigit ?

In parseInteger, you used the character '0' instead of its ascii code (48).

I assume this is std::isdigit ?

Functions from cstdlib are in the global namespace, although those symbols are imported into std as well.

whaeck · 2017-11-27T18:20:04Z

src/disco/FixedWidthField/Real/src/parseInfinity.hpp

-  default:
-    return false;
+    success = 
+      ( toupper( *it++ ) == 'I' )


I assume this is std::toupper ?

whaeck · 2017-11-27T18:23:11Z

src/disco/FixedWidthField/Real/test/read.test.cpp

@@ -10,10 +10,24 @@ SCENARIO("Real read", "[Real], [read]"){
    test_strings{ {"      +123"}, {"     123.0"},
                  {"   123.123"}, {"      -123"}, 
                  {"      1E99"}, {"    1.E+99"}, 
-                  {"     1E-99"}, {"    1.0-99"} };
+		  {"     1E-99"}, {"    1.0-99"},


I am not seeing 1.0+99 in the test.

Nevermind, I saw one in the pi(e) test. I suggest to still add it here as well.

900549f Merge pull request #15 from njoy/feature/python3_to_python a348df3 Merge pull request #16 from njoy/feature/WindowsyLinux 24cd40e Added subobject linkage error exception in common flags. b3d6955 Merged. f22bae2 Adding Windows options for Cygwin and MinGW. 41786f5 Added check in CMakeLists.txt generated by cmake.py for python version. e72a44f Added version check of Git in constructed CMakeLists.txt. 0748844 Windows/Cygwin testing. 597ff74 Changing python3 to python. ca6c308 Added MinGW and CYGWIN environments to metaconfigure. ff293c6 Update cmake.py 6bda6e9 Update configuration.py 69647ab Update cmake.py 730615a Update index.md d584ae2 Merge pull request #10 from njoy/docs 8582ee2 Beginning of metaconfigure documentation. 10fad1f Add 'docs/' from commit '9a9fd02407b5cedbb0d1dd837143187e96dbdea8' 3f39eb5 Merge pull request #9 from njoy/README 7f027c0 Fixing issue related to Python3. 3aa9553 strictness project specific 8bd1daa subproject to is_subproject 3356ae0 executable installation only when not subproject 7263e94 fix for libraries a3e5adb constrain configuration types 470c254 directory installation correction 48ccecc regex error 44886a1 spacing in linker flags a7c72a9 Updating README following pull request comments. cc89730 bugfix 6457dca rounding math removed and coverage extension 6c360a0 correction 35a3091 coverage in tests necessary for header-only libraries edd43bb local changes 4b4f943 Clarifying need for include path. 156cb72 Being more specific 2e5a0a7 Finished with updating commands. 47cd119 Working on README. 1dce0f6 Adding README file. cbd8068 apple clang 8fe650a clang compliant debug flags 06fc1dc config e6fb36d more updates 982fa8b update b07c58c split d840594 directory installation 5a03ea0 whitespace 4103d38 dependencies 75d0f1d subproject linking bug 5e9ac83 missung header extension 2657253 ignore path default c486ab6 only verify version if version is available 9507100 bug 65038bd wrong order cd179c1 bug 2cb9403 precedence d9b76f1 bug in degenerate case e86e3fd need more for subproject 5faa5b8 another bug 2493440 circular d854e3e bug 50877ee correction d6bac61 underscore keys f93301c tweaks 2ff8fc0 external projects are hard 5bd040d Merge pull request #8 from njoy/refactor/generalize ebb26a2 consider subprojects 0fd9a10 Merge pull request #6 from njoy/refactor/generalize 0d9c813 manual merge 22daf46 Merge pull request #5 from njoy/refactor/generalize d19af43 njoy2016 testing/debugging 1527f74 debugged using njoy 64aca82 refactor dce72af Merge pull request #3 from njoy/feature/collect_subprojects d51b28a long lines 3164c69 don't build subproject executables bae56cd correction c10fdbd Correctly handle multiple test resources f7c0ece offline script 9a9fd02 Adding some information about not pushing to this remote. 7e673cc Fixing markup typo. 75d2869 Adding LA-CC to footer. c3e5e2e Fixing where the header href points to. 6d7eeee Removing base-url from navigation href. e4b5ce8 better fortran module organization fbd3876 removed project specific files 332563f Updating navigation links. e6a92f3 Moving where description is derived. 1ed4817 Fixing link to LICENSE file. 11d8cfb Making some changes to the look. f46cfc3 Updating config to [in/ex]clude some files. 0d23c3c Making link to license file in footer. a048936 Updating URL to Developer's Guide. 169ecf5 Removing projects.yml from this repository. a7dbded Changing location of DevelopersGuide. 52c6579 Adding data for NJOY projects. 15c529c Renaming README so it doesn't conflict. 06e582b Adding Gemfile. 2eb701e Adding all the basic Jekyll stuff. 13d0fbb Initial commit aad4441 testing work ef3f9ba bug 6dec9f8 namespaces considered in name of test in description git-subtree-dir: metaconfigure git-subtree-split: 900549fd9e2604dbfa2161795a5c832d0bf4bd46

white space improvements

1e47ba8

apmccartney requested a review from whaeck November 23, 2017 01:45

whaeck approved these changes Nov 27, 2017

View reviewed changes

apmccartney merged commit 1abb028 into master Nov 28, 2017

whaeck deleted the feature/whitespace branch September 22, 2022 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

white space improvements #8

white space improvements #8

apmccartney commented Nov 23, 2017

coveralls commented Nov 23, 2017

whaeck Nov 27, 2017

apmccartney Nov 28, 2017

whaeck Nov 28, 2017

whaeck Nov 27, 2017

apmccartney Nov 28, 2017

whaeck Nov 27, 2017

apmccartney Nov 28, 2017

whaeck Nov 27, 2017

apmccartney Nov 28, 2017 •

edited

Loading

whaeck Nov 28, 2017

whaeck Nov 27, 2017

whaeck Nov 27, 2017

apmccartney Nov 28, 2017

whaeck Nov 27, 2017

whaeck Nov 27, 2017

whaeck Nov 27, 2017

white space improvements #8

white space improvements #8

Conversation

apmccartney commented Nov 23, 2017

coveralls commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apmccartney Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apmccartney Nov 28, 2017 •

edited

Loading