# Tag Info

17

Is there anything that can be simplified or made even cleaner? Yes, don't invoke the Java compiler at runtime. From your examples in a comment: DynamicException.Create($"ExtractMethodName(memberName)}NotSupported", ...) From an example on your earlier post (in C#) throw ("SettingNotFoundException",$"Setting {fullName.ToString().QuoteWith("'")} not ...

16

I see a number of things that may help you improve your program. Since the existing review covered a lot of good points, this review will cover the parts not already mentioned. Use the correct form for main There are exactly two allowed version of main, according to the standard, and yours isn't one of them. This code has this: int main(int argc, const ...

15

One small comment about your API. public void Tokenize() I would expect a Tokenize method to return a stream (IEnumerable) of Tokens rather than modify state. This should be a pure and idempotent function in my opinion.

15

Allocations can fail Don't use buffer until we know it's not null. (And no need to multiply by sizeof (char), since that's automatically a no-op). Please remember to free() it too (at least as an option). That reduces false positives with Valgrind's memory checker. I/O operations can fail Always check that your I/O succeeds. Consider this scenario: we ...

12

#ifndef __JMCOMP_LEXER_H__ That is a symbol reserved for use by the implementation. Writing your this in a way that mimics what you see in the compiler-supplied headers is exactly wrong! For this and other reasons I suggest leaving that out completely and just use #pragma once. If you ever find a platform were this pragma is not supported, a script can ...

11

Dayum, there's some static around here. Why not make it an instance class? What you have here is a workflow, where methods have a specific order. This is clearly seen in your Main method. The workflow usually have a state, and static stuff is supposed to be stateless. So I really think an instance class would be good! Doing this would also mean you wouldn'...

11

Your code looks very clean and organized. Only some comments (without understanding the code in depth) You would probably use the regex option RegexOptions.Compiled to improve performance The Tokenize method has lots of similar code fragments. You could use a list of SyntaxKind elements like you did with LiteralTokens. Here you call MatchesDefition twice: ...

11

Surprising This is a gross violation of the Principle of Least Surprise. The only possible reaction I could have seeing this in the wild would be, "What the heck?" This is not a normal way of using exceptions. New developers approaching the code are likely to have a similar reaction. One particular thing that would trip me up and force me to spend time ...

10

Just one small point that @200_success probably left out so I can say something too: when matching something against $_, for example in if ($_ =~ /[0-9]+/) { ... }, you can simply omit the $_: if (/[0-9]+/) { # ... } It's "the default input and pattern-searching space". Read more about the$_ variable in man perlvar or on perldoc.perl.org. The main ...

10

tokenize The tokenize subroutine could be simplified: sub tokenize { my ($code) = @_; die "Invalid Arguments" unless defined$code; return $code =~ m!\d+|[-+*/.]!g; } Changes include: Shorter parameter name One-line validation Use global match in list context to produce a list of all matches Simpler regex that avoids leaning toothpick ... 9 First, a trivial optimization: there is no reason for dyn to be dynamic: all it does is to make the Target invocation dynamic, which here only makes it slower. On my computer, this makes Get/Set about 25 % faster. Another possibility is to use Expression instead of CallSite. Since Expression is not as dynamic as dynamic, it can compile to simpler code. For ... 8 The keywords, separator, comments, and operators arrays could be static readonly, so that they don't need to be re-initialized for every instance of a LexicalAnalysis class you create; the type would probably be better off as LexicalAnalyzer though. The analyzer performs the analysis, but it is not "the analysis". Your private members members are all ... 8 Just a few very minor points... There's a couple places where you do this sort of thing: string brainfuck = string.Empty; using (StreamReader sr = new StreamReader(filename)) { brainfuck = sr.ReadToEnd(); } Which could be more easily written as: var brainfuck = File.ReadAllText(filename); StringBuilder's append methods return the StringBuilder so ... 8 if ((OptimizationLevel & OptimizationLevel.Level1) >= OptimizationLevel.Level1) { if (lastSymbol != symbol && lastSymbol != TokenSymbol.None && (lastSymbol == TokenSymbol.Decrement || lastSymbol == TokenSymbol.Increment || lastSymbol == TokenSymbol.MoveLeft || lastSymbol == TokenSymbol.MoveRight)) { Lines.AddRange(... 8 The compile concept produced incorrect results for me. When I run your code the template does not produce the correct results. For the input parameters: final Map<String,String> parms = new HashMap<>(); Stream.of("USER_NAME", "USER_PHONE", "USER_EMAIL", "LOGIN_URL") .forEach(tag -> parms.put(tag, tag)); I would expect the input String: ... 8 A few other things: cursor += 2; and cursor++; are all over the place in your Tokenizer. This looks very error-prone. You need a class that will move cursor automatically when you read a substring. Something similar to StringReader, but designed to better fit your task. Maybe: //instead of returning strings you can return a complex object, //that would ... 8 Another point that hasn't been fleshed out by other reviewers is the generated python code. list is a built-in function in python - by calling your list list you are overriding it. That's generally considered bad form in the python community, mostly because someone could be stung if they try to use the list constructor later in the code. So instead, let's ... 7 [DebuggerDisplay("[Number: Value={Value}]")] I have a new favorite attribute :) First time I encounter C# code using a DebuggerDisplayAttribute - and I must say it makes an awesome feature, especially for that kind of code. // Variables evaluate to the Name // mapping in the current environment. public Value Evaluate(Dictionary<string, Value> env) {... 7 The code looks OK for what it does so far, but there are some things you might want to do to improve it: Always use {} for production rules It's not technically wrong to simply have printf(...) to the right of a rule, but when your lexer gets more complex (and when you start also using a parser) you may find it easier to troubleshoot if you always use {} ... 7 I'm not a huge fan of reading all input in one one go and transform it in one and then write it in one go. It seems clumsy and will not scale properly (although in the given case it's probably not going to be a problem). The basic structure I'd go for is this: open my$ASM, '>', "$output.asm" or die "Cannot open file to write '$output.asm'\n:$!"; ... 7 Some simplifications You can initialize the symbols dictionnary in one instruction: # Memory map symbols = { "IN0": 0xfff8, "IN1": 0xfff9, "OUT0": 0xfffa, "OUT1": 0xfffb, } The format template string, when applied to a number, can take a base specifyier. So '{:b}'.format(x) will pretty much return the same thing than bin(x) except without ... 7 Your TargetCodeGenerator's generateAST has multiple abstraction levels embedded into one method. And it's a long method. It has all of these details that are pretty complex - when I see functionality like dataSectionNode.addChild(ASTNode.newWithChildren(new DefineByteExpression(), Arrays.asList( ASTNode.newWithChild(new IdentifierExpression(), ... 6 It's not guaranteed that$TMP_PROGRAM_FILE is on the $PATH, so you need to either set the$PATH or use an absolute path for \$TMP_PROGRAM_FILE. You have problems with mktemp. -t option: It means different things on GNU/Linux mktemp(1) and Mac OS X mktemp(1). On GNU/Linux: -t interpret TEMPLATE as a single file name component, relative to a ...

6

You can use verbatim strings to reduce the slash chaos: { SyntaxKind.RootOperationToken, new Regex(@"\/\/") }, You can also use IReadOnlyDictionary to prevent your dictionaries from being modified. The readonly flag tells that no one can write something like Keywords = null but currently it still can be modified, e.g. Keywords.Clear();. private ...

6

I'm mostly reviewing the code-gen choices, not the style / implementation of the compiler itself. The compiler itself mostly looks fine in the parts I've skimmed over (looking for how it uses the format strings). The design is pretty clear and easy to dive into, and the C is well-formatted. It could be simpler and/or more efficient in some cases. (e.g. ...

6

First, you tackled a tough problem and you obviously put a lot of effort into it. The code is well structured. The organization of the code seems fine. My code contains occasional free() statements, but my primary memory management strategy was not to release any memory, as the program works only for a couple of seconds. Two seconds is a lot of time, ...

6

You'll run out of stack memory if you recursively call run() at so many places. You can remove those. You should consider opening the file with a another context manager to avoid having the file handle kept open. Consider restricting the try block to only the part of code you expect to fail.

6

A lot of could be written in a more functional style. That means that functions return the result of their execution instead of printing it. It makes your code more readable, more composable and generally easier to reason about. Also, docstrings at the beginning of all the function would help immensely in determining their purpose. For the details like ...

5

I will give a quick overview of some aspects that I found at a first glance. Unfortunately, I could not spare the time to do an in-depth analysis of the cross-plattform capabilites and/or thorough testing of your program. As I find your idea very interesting, I maybe check back later with more feedback. Edit: Updated requirements for proper import and added ...

5

return isalpha(c) || c == '\'' || c == '?' || c == '+' || c == '-' || c == '*' || c == '/' || c == '^' || c == '=' || c == '>' || c == '<' || c == '~'; This could be rewritten much more readably as return isalpha(c) || (c != '\0' && strchr("'?+-*/^=><~", c) != NULL); A good compiler will produce the same code for both. (Neither GCC nor ...

Only top voted, non community-wiki answers of a minimum length are eligible