Good stuff!

Thank you!
I hope someone familiar with the original code would find time to review the changes.

Indeed many thanks for this, especially considering we all know this is not the best maintained part of the code base. I will have time for a closer look probably tomorrow.

But indeed,

EOL chars may appear in any tokens

this is indeed the main reason why there are these std::string allocations in the parser code base now. First of all, my impression is that random new lines within tokens will break many of the parsers out there. Also, numbers of more than 63 digits I find a bit of a pathological example. But how about the other option you mention, stripping all carriage returns and newlines before tokenization, you should be able to do this in-place without too much overhead am I right? Another monstrous option of course is to provide drop in replacements for ato_() that skip over newline characters there, surely there must be reliable sources for this we can adapt. So to summarize:

• Allow newline to break parser
• Use buffer on stack (This PR)
• Use dynamic buffer (master)
• Dynamic static buffer (beware multi-threading).
• Pre-process to remove newlines (beware BUF_SIZE macro)
• Alternative ato_() implementations that ignore newlines
• More...?

Any thoughts? We can defer a final decision to the 0.6 release and stick to this PR for now.

Regarding the exceptions: 5 years ago many IFC files were semantically and syntactically invalid in various ways and we felt that a very permissive parser, one that would just keep churning along until some exception was thrown, was most desirable. Still at this time, many IFC files have their subtle issues, such as issues with cardinalities. I am not entirely sure how this relates to your proposal of an assertion macro. I do agree that either a strict parsing mode or an earlier indication of whether the file conforms to at least the subset of the schema you are interested in would be valuable and would also keep the code (especially the geometry interpretation which is full of try catch blocks for this reason) cleaner.

Stripping all EOLs before parsing is trivial to do, but there is one problem.
It seems that there exists a streaming mode in IfcSpfStream, it can be enabled by defining a macro BUF_SIZE. And it is not possible to add input preprocessing with this feature enabled. Well, at least it is not easy to do: it is necessary to maintain correspondence between real positions in input file and virtual positions in the resulting preprocessed buffer.
Other from that, it should be simple and relatively fast. Moreover, it can perhaps simplify parsing even further, especially the TokenString function.
BTW, IfcPlusPlus strips all EOLs and comments before parsing.

I also feel that newlines inside numbers is never generated and not supported by many programs. But a standard is a standard, unfortunately. A very long integer is indeed a highly unlikely case, but a very long float seems to be possible.

As for writing custom atoi and atod equivalents, I was also considering this option. But it is quite a lot of nontrivial code to write and debug.

Another option is to create a single std::string object, and use it as a temporary location for all the tokens (and shrink only when finished). It can be trivially achieved by making a static local variable in IfcParse::GeneralTokenPtr function. However, this would immediately break multithreaded parsing. This temporary buffer can be placed into some object like IfcSpfStream (or IfcFile), then it would work safely under condition that no IfcSpfStream object is used by several threads simultaneously.

Yep, good point. This BUF_SIZE has not been used as the intermittent seeking was very detrimental to performance. However, with this pull request and the most prominent token values actually represented in memory, it might be worthwhile to reconsider this streaming parsing.

Of course there are good examples of atoi and atod, e.g [1] we don't necessarily need to reinvent the wheel. But I too am not very much in favor of this option.
[1] ftp://ftp.irisa.fr/pub/OpenBSD/src/sys/lib/libsa/strtol.c

Multi-threaded parsing seems quite far out to me still. I am also fine with static buffer that can grow. I am sure we can we can somehow instantiate different buffers per thread later if that ever becomes imminent.

I have implemented the last way: added a _tempString field to IfcSpfLexer, which is used in several cases as local storage. So numbers can have unlimited length now. It also allows to handle keywords without allocations.

P.S. I'm sorry for the EOL style: it seems that I have messed it in the first bunch of commits.

Thanks again. I think that's a good approach.

Now, I ran IfcConvert compiled with this PR and master and compared the perfomance. It's not really conclusive or anything as it is hard to eliminate e.g hard drive caching of the file. It does seem overall the performance increases nicely. IfcConvert is not the best executable to test this PR as most of the time is spent inside Open Cascade modules. Memory usages increases a bit too, which is not unexpected. I wonder if we can reduce it a bit though. One potential option would be to store aggregates not as separate tokens, but already aggregate them into vectors at the parsing phase.

With this python script I isolated a bit more on the performance of the parser

import sys, time, ifcopenshell
li = []
t0 = time.clock()
f = ifcopenshell.open(sys.argv[1])
t1 = time.clock()
for pt in f.by_type("IfcCartesianPoint"):
    li.append(pt.Coordinates)
t2 = time.clock()
print t2 - t1, t1 - t0

The differences are quite significant:

|        | t2-t1 | t1-t0 | 
| pr     | 6.7   | 12.4  | 
| master | 9.5   | 14.6  |

I'll be ready to merge it in soon.

With regards to the exception / assertion, I agree things were not ideal. However, by not throwing exception, invalid data will seep through the cracks now. For example:

#11=IFCGEOMETRICREPRESENTATIONCONTEXT('Plan','Model',3,.T.,#10,#6);
                                                        ^ Precision should be REAL

f.by_id(11).Precision will now happily return *(float*) (&0xcccccc01) (on a msvc debug build, otherwise 3 bytes of garbage). Therefore I vote for re-enabling the exceptions in the asFloat() and similar functions until we have something better, as that is what the rest of the codebase is currently expecting.

I agree a check on construction is preferable, for example in the IfcSchema::SchemaEntity() call. As that is were the untyped IfcParse instance is associated to a typed instance from the schema. If it fails that check, the instance would simply not be inserted in the maps of IfcFile. But that would be for a different PR I would say. The question then still remains what checks to do: attribute types, cardinalities or potentially even inverse cardinalities and where rules, the latter two being astronomically more complicated of course.

In case I don't hear back I will simply add the exceptions and otherwise merge as is.

I agree with you.
I was under impression that caller always checks the type with isFloat function before calling asFloat. But it turns out that it is not so. Committed a small fix.

As for reducing memory consumed, I think implementing special treatment for tessellation entities should improve it a lot (along with performance). I'm not sure: what's the best place to discuss this topic?

Ok, perfect, thanks.

Best place to discuss would be to open an issue here on github and label it improvement, that way we have a reference to the discussion when it gets implemented.