Fix #172 #342
Fix #172 #342
Conversation
sparkprime
force-pushed
the
fix_import_desugar
branch
from
3dca1ba
to
9a19c6b
Compare
| @@ -138,7 +138,8 @@ <h2 id="lexing">Lexing</h2> | |||
| first subsequent line that does not begin with <i>W</i>, and it is an error if this line does not | |||
| contain some optional whitespace followed by <code>|||</code>. The content of the string is the | |||
| concatenation of all the lines that began with <i>W</i> but with that prefix stripped. The line | |||
| ending style in the file is preserved in the string.</li> | |||
| ending style in the file is preserved in the string. This form cannot be used in | |||
There was a problem hiding this comment.
A test for that would be nice.
core/ast.h
Outdated
| /** Represents import "file". */ | ||
| struct Import : public AST { | ||
| LiteralString *file; | ||
| Import(const LocationRange &lr, const Fodder &open_fodder, LiteralString *file) | ||
| // Must be a LiteralString. |
There was a problem hiding this comment.
Then why not keep it *LiteralString?
(Also it doesn't compile on Travis and I think it's because of that).
There was a problem hiding this comment.
Because the desugarer visitor allows editing the AST *&, so everything has to be an AST *.
There was a problem hiding this comment.
True of CompilerPass too, actually.
There was a problem hiding this comment.
well, yes, but it can be handled while keeping Import::file a *LiteralString. Instead of:
desugar(ast->file, obj_level);
you could:
AST *file = ast->file;
desugar(file, obj_level);
ast->file = dynamic_cast<LiteralString *>(file);
And it may be better, because:
a) It's more explicit - it clearly states that it needs to be LiteralString, but in this case desugarer could potentially put something else there.
b) In general a little bit nicer data structures and a little more code within some functions is better than the other way around, imo.
sparkprime
force-pushed
the
fix_import_desugar
branch
from
c6cd6ec
to
daf563c
Compare
sparkprime
force-pushed
the
fix_import_desugar
branch
from
daf563c
to
dc7798e
Compare
|
LGTM |
@sbarzowski