Could m_subs be a vector of unique_ptrs? If it can, it should, since from what I can tell, we're only iterating over its contents later on, so no need for this to be a sharted_ptr.

Maybe that's possible to change that, but it would be an unrelated change. But if it makes sense, we could create another PR changing this.

How non-strict is jsonParse()? If the difference is just the null, it's fine, but I have a feeling that this will affect also some other aspects of parsing that we'd rather keep strict :)

Not restrictive at all, it uses the default settings. Please see setDefaults() and strictMode() for a comparison.

But we could change the strictRoot field in the strict settings to false, which will allow us to accept Json::null, however we need to double check possible consequences of this. The commit 772ff3e was just an example of the issue.

I reverted the changes anyway ;)

-  (*settings)["collectComments"] = true;
-  (*settings)["allowComments"] = true;
-  (*settings)["allowTrailingCommas"] = true;
-  (*settings)["strictRoot"] = false;
+  (*settings)["allowComments"] = false;
+  (*settings)["allowTrailingCommas"] = false;
+  (*settings)["strictRoot"] = true;
   (*settings)["allowDroppedNullPlaceholders"] = false;
   (*settings)["allowNumericKeys"] = false;
   (*settings)["allowSingleQuotes"] = false;
   (*settings)["stackLimit"] = 1000;
-  (*settings)["failIfExtra"] = false;
-  (*settings)["rejectDupKeys"] = false;
+  (*settings)["failIfExtra"] = true;
+  (*settings)["rejectDupKeys"] = true;
   (*settings)["allowSpecialFloats"] = false;
   (*settings)["skipBom"] = true;

I see that there are some important differences. For example rejectDupKeys does not sound like something we'd like to disable. And even things like comments or trailing commas would be something we could not roll back later in a non-breaking way when we finally switch to a better maintained JSON library.

Using strictMode() with just the strictRoot flag flipped would probably be acceptable, but still, this is really something better done in a separate PR, because here there are too many other concerns mixed in. We do need to consider whether the new JSON parser will allow that or not.

These code snippets should not be printed. The input file (<stdin>) is listed as one of the sources, which makes no sense but makes the compiler use it as the source file. The thing is, in the EVM asm import mode we never have access to sources so while printing locations makes sense, the snippets should be disabled. I.e. of all the values supported by --debug-info (ast-id location snippet) only location should be allowed. And the default should be different.

Honestly, I'd be perfectly fine leaving this as is, since it's not checked by check_style.sh, and we'll eventually move to clang-format, where we'll have to auto format the whole project anyway. At your discretion then.

when clang-format? haha

Again, just pointing out - feel free to ignore (or not if Kamil sees this).

This is basically just a nested for loop - so a bit wasteful; wouldn't it make more sense to make parsedSourceList a std::set<std::string> and use parsedSourceSet.count(sourceName.asString()) instead, and then just copy it to a vector when/if needed, i.e. std::vector<std::string> parsedSourceList(parsedSourceSet.begin(), parsedSourceSet.end())?

Wouldn't that destroy the order though? The order here is very important.

But other than that, yeah, that would be more efficient for long lists.

Yes, it would, in which case we couldn't simply copy content over from the set into the vector, but could none the less use the same approach (i.e. using a set for a presence check, which technically wouldn't be any less efficient.

In that case it would be better to first load all items into a vector and then only use the set as a temporary helper to check if items are unique. Even better, wrap that in a reusable isUnique() helper. Or maybe such a helper even already exists in boost?

But IMO this is also good enough as is, given that the feature is experimental and we gave some other inefficiencies and suboptimal things a pass too. This could be improved to the follow-up refactor PR.

This is a bit convoluted - can't we just get dataItem directly from dataIter?

I thought the same thing and apparently we can't. Though I only briefly looked at how the iterator is defined. I didn't see a member for it, but maybe you can find some mechanism for it if you look closer. In any case, I gave up myself on this because there were too many other things to adjust here.

Dereferencing the iterator should work, no?

Maybe. Feel free to check and suggest a variant that will work :)

Looks like it works. In any case, regarding your latest comment about priorities - I wouldn't worry about this too much, and I'd prefer not to push to the branch in case @r0qs is actively working on (which I'm assuming he is). So we can leave it for another time, unless of course, he notices this comment and does it himself :)

Pushing should not be a problem as long as you use --force-with-lease and only push new commits without modifying any existing ones or rebasing the whole branch. Then for @r0qs it would be a simple git rebase -i origin/import-asm-json-updated (and maybe some conflict resolutions, but this is a small localized change and everyone should be used to resolving conflicts by now anyway :)).