Bug #360

Grompp error due to incorrect reading of itp file

Added by Anonymous over 10 years ago. Updated over 10 years ago.

Erik Lindahl
Target version:
Affected version - extra info:
Affected version:


This line in an itp file gives the following error

No default G96Bond types error:
31 32 2 gb_18; C-O ether bond

Works fine with a space beween the bond code and the semicolon:
31 32 2 gb_18 ; C-O ether bond


Associated revisions

Revision 081e595d (diff)
Added by Teemu Murtola over 10 years ago

gmxcpp fixes (bug #360 plus other fixes).

The preprocessor is no longer as picky in the input it accepts, making
it behave much more predictably when users expect cpp-like behavior.
Defines are now replaced even if they are not delimited by spaces, as
long as they form complete identifiers, and indendation in preprocessor
directives is handled gracefully. As a side effect, preprocessor
directives can also be commented out now.

Revision 5485b5b5 (diff)
Added by Teemu Murtola over 10 years ago

gmxcpp fixes (bug #360 plus other fixes).

(backported commit 081e595d49fec1f5afdbf075af88a01eea205fb3)
(cherry-picked commit aa6789e814100b3ea2cf5921bd5724462b6c2dc4)


#1 Updated by Berk Hess over 10 years ago

I don't know if this it a bug or if we (should) require
a space before the semi-colon.


#2 Updated by Teemu Murtola over 10 years ago

The cpp replacement in grompp does several things a bit differently compared to a real preprocessor, this being one of them (another being that it doesn't remove defines that don't have a value). I think it would be better to be as compatible with cpp as possible, because the syntax certainly suggests it would work as cpp, and this would mean not requiring a space. Also, in other cases grompp works fine without a space before a comment.

I think one of the main problems is that many of these things can go unnoticed (or result in mysterious errors, as the one reported in this bug), because if grompp encounters anything it doesn't understand, in most cases it just silently ignores the end of the line.

The fix for this particular bug should be straightforward, just replacing is_blank_end() in src/gmxlib/gmxcpp.c with something more appropriate. Or, the comments could be removed during the cpp pass, but I don't know whether this is a good idea because the comment syntax is not the same as in C.

#3 Updated by David van der Spoel over 10 years ago

on line 87 should do the trick.

I think this is a bug since it can give the wrong result as Teemu pointed out.

Adding this statement compiles in the head branch, but I can not seem to do anything useful in the release-4-0-branch. Teemu, could you please try to fix this one?

#4 Updated by Teemu Murtola over 10 years ago

I think the best solution would be to change the is_blank_end() to return, e.g.,
!(isalnum(c) || c == '_')
to avoid any problems of this kind in the future. This does have a slight effect on how #-directives are handled because they are also matched using strstrw(), but I don't think it will make things any worse: even now, incorrectly formatted #-lines seem to be able to cause really weird behavior... This is because, e.g., #define is matched against the whole line, but if it's found, the code assumes that it matched in the beginning without actually checking. This is probably worth fixing as well.

#5 Updated by Teemu Murtola over 10 years ago

I fixed several things related to the preprocessor both in git master and release-4-0-patches, including this bug. The preprocessor still doesn't replace defines that don't have a value, but otherwise it works much more similarly to cpp, and this difference should not currently matter. I also added a gmxdump option to test the preprocessor (this seems to have been there at some point). There's also a minor memory leak that I noticed (basically, the gmx_cpp structures never seem to get freed), but didn't have time to figure out whether it can simply be freed in cpp_close_file(), or whether it is actually used after it somewhere in the code.

Also available in: Atom PDF