In D100955#4054569, @thakis wrote:

Can you say a few words on _why_ this is emitted in only Objective-C? What's missing for Objective-C++? I was surprised that this warning fired in a .m file but not in a .mm file.

I guess the only possible scenario that I can imagine here for including these files is tests. Tests for some code tools dealing with patches.
Otherwise, 100% reasonable.

Ooooh, delicious! Diving in!

@aaron.ballman a gentle ping

Finally I had a chance to come back to this patch.
@aaron.ballman what do you think about it? I tried to address your notes and implemented both features under one attribute.

Join 'suppress' and 'analyzer_suppress' attributes.

Now it looks good! Thanks again!

In D107636#2931302, @steakhal wrote:

Seems reasonable to me. Let's wait for someone else as well.

Now it captures (and makes bold) one extra arrow from the previous note.
Correct example attached!

Oh, wait! I found a bug!

In D107366#2926662, @ASDenysPetrov wrote:

In D107366#2926343, @NoQ wrote:

Works in Firefox on macOS as well!

Great!
@vsavchenko , any suggestions?

Can you please also attach an HTML file just to verify that it works?

Looking great!
I have a couple of nit picks and I kind of want to check that it doesn't affect the performance on a different set of projects as well.

That looks interesting!
Can you please add tests, though?

In D92928#2913552, @ASDenysPetrov wrote:

Or, we can find another symbiotic way. You can make changes the way without painfull part of thinking about IE. And I will prepare the next patch adjusting it. Thus, revisions would be smaller. That's would be easier for you to test all the things before the load. I will take a charge for IE part on my own and prepare a new revision.

Awesome, I have nothing to add at this point!
Let's still wait for @aaron.ballman to check it.

Great job! It looks good, but I have a couple of minor tweaks to suggest.

Looks good! Thanks!

Oh, I didn't accept it? Sorry!

It looks reasonable to me, but I can't say that I'm an expert here, and I'm definitely not a frequent z3 user.

Awesome! Thanks!

Can we please land the fix?

In D103440#2891915, @manas wrote:

Here is the proof of correctness of the algorithm using Z3: https://gist.github.com/weirdsmiley/ad6a9dbf3370e96d29f9e90068931d25

I want to say that I really appreciate the effort you put into finding all the workarounds for IE, but it makes adding new features here incredibly painful because IE doesn't seem to support anything. And the majority of developers (on Linux and on MacOS) have literally no way to test it. What we gain from supporting IE for non-existing users, we lose in the ability to actually improve this code!

Maybe I'm missing something, but do we really need to care about IE? The last version was released in 2013, and even Microsoft itself stops supporting IE. Why should we care? Is there anyone who uses old deprecated browser that is not maintained? classList thing was here for almost 4 years and no one seemed to care. Am I missing something here?

Great, LGTM!
But let's wait for @xazax.hun anyways

Also, I tested this fix on a set of open-source projects and I don't see any problems.

Ah, I see now! I think we could've put together a somewhat easier test knowing what's wrong, but it's not important at all.
Thanks for addressing this issue!

In D106136#2889610, @martong wrote:

In D106136#2883707, @vsavchenko wrote:

In D106136#2883100, @martong wrote:

Thanks for taking your time to take a look. And I accept your statements. Nevertheless, let me explain my motivation.

I thought that a class is trivial if it has only one member. And it seemed perfectly logical to not store equivalence classes with one member. I.e a equals to a does not hold any meaningful information it just takes precious memory. When we add a new constraint we take careful steps to avoid adding a new class with one member. However, when remove dead kicks in, suddenly we end up having classes stored with one member, which is a somewhat inconsistent design IMHO. Perhaps some better documentation could have helped.

Representative symbol gives its equivalence class an ID. We use this ID for everything, for comparing and for mapping. Since we live in a persistent world, we can't just change this ID at some point, it will basically mean that we want to replace a class with another class. So, all maps where this class participated (constraints, class, members, disequality) should remove the old class and add the new class. This is a huge burden. You need to carefully do all this, and bloat existing data structures.

Trivial class is an optimization for the vast majority of symbols that never get into any classes. We still need to associate constraints with those. This is where implicit Symbol to Class conversion comes in handy.

Now let's imagine that something else is merged into it. We are obliged to officially map the symbol to its class, and the class to its members. When this happens, it goes into the data structure FOREVER. And when in the future, with only one member, instead of keeping something that we already have from other versions of the data structure, you decide to remove the item from the class map, you actually use more memory when it was there! This is how persistent data structures work, different versions of the same data structure share data. And I'm not even talking here about the fact that you now need to replace the class with one ID with the class with another ID in every relationship.

You can probably measure memory footprint in your example and see it for yourself.

Yeah, makes sense. Thanks for taking more time for further explanations. Still, IMHO, the definition of a trivial class needs a clear written documentation in the code itself, so other developers can easier understand and maintain this code. I am going to create an NFC patch that summarizes this discussion in a comment attached to the isTrivial function.

In D106136#2883100, @martong wrote:

Thanks for taking your time to take a look. And I accept your statements. Nevertheless, let me explain my motivation.

I thought that a class is trivial if it has only one member. And it seemed perfectly logical to not store equivalence classes with one member. I.e a equals to a does not hold any meaningful information it just takes precious memory. When we add a new constraint we take careful steps to avoid adding a new class with one member. However, when remove dead kicks in, suddenly we end up having classes stored with one member, which is a somewhat inconsistent design IMHO. Perhaps some better documentation could have helped.

Thanks for working on it, but it is a quite large change that I don't get the motivation for (it doesn't even fix the recently found bug).

Yes, let's do this! Thanks for addressing it!

In D103096#2877818, @ASDenysPetrov wrote:

Made ignoreCast non-virtual.
P.S. IMO, this change is not something that can be taken as a pattern, though.

// 1. `VisitSymbolCast`.
// Get a range for main `reg_$0<int x>` - [-2147483648, 2147483647]
// Cast main range to `short` - [-2147483648, 2147483647] -> [-32768, 32767].
// Now we get a valid range for further bifurcation - [-32768, 32767].

OK, thanks for putting a summary. I now got a good idea why you need both.
At the same time, take a look at D105692. I'm about to land it and I think it's going to be useful for you.

Fix comments

Fix comments

I'll allocate some time to get into your summary, but for now here are my concerns about SymbolRangeInferrer and VisitSymbolCast.

In D105421#2873458, @RedDocMD wrote:

Removed stupid mistakes

In D103096#2867441, @ASDenysPetrov wrote:

@vsavchenko

Why did you write it this way!?

I want the map contains only valid constraints at any time, so we can easely get them without traversing with all variants intersecting with each other. I'm gonna move updateExistingConstraints logic to VisitSymbolCast. I think your suggestion can even improve the feature and cover some more cases. I'll add more tests in the next update. Thanks!

In D103096#2867136, @ASDenysPetrov wrote:

@vsavchenko

I still want to hear a good explanation why is it done this way. Here c is mapped to (char)x, and we have [-10, 10] directly associated with it, but we also have (short)x associated with [8, 8]. Why can't VisitSymbolCast look up constraints for (short)x it already looks up for constraints for different casts already.

Hm, you've confused me. I'll make some debugging and report.

In D103096#2867104, @ASDenysPetrov wrote:

Generally, with this patch we kinda have several constraints for each cast of a single symbol. And we shall care for all of that constraints and timely update them (if possible).
For instance, we have int x and meet casts of this symbol in code:

int x;
(char)x; // we can reason about the 1st byte
(short)x; // we can reason about the 2 lowest bytes
(ushort)x; // we can reason about the 2 lowest bytes (in this case we may not store for unsigned separately, as we already stored 2 bytes for signed)

That's like we have a knowledge of a lower part of the integer. And every time we have a new constraints, for example, for (short)x; (aka 2 bytes) then we have to update all the constraints that have two bytes or lower ((char)xin this case) as well to make them consistent.

In D103096#2867021, @ASDenysPetrov wrote:

In D103096#2866730, @vsavchenko wrote:

In D103096#2866704, @ASDenysPetrov wrote:

@vsavchenko

That's not the question I'm asking. Why do you need to set constraints for other symbolic expressions, when SymbolicInferrer can look them up on its own? Which cases will fail if we remove that part altogether?

I see. Here is what fails in case if we don't update other constraints:

void test(int x) {
  if ((char)x > -10 && (char)x < 10) {
    if ((short)x == 8) {
      // If you remove updateExistingConstraints,
      // then `c` won't be 8. It would be [-10, 10] instead.
      char c = x;
      if (c != 8)
        clang_analyzer_warnIfReached(); // should no-warning, but fail
    }
  }
}

In D103096#2866704, @ASDenysPetrov wrote:

@vsavchenko

Can you please explain why you do the same thing in two different ways?

Great! Thanks for addressing all of the comments!

Great, thanks for addressing my comments! I still have a couple of minor suggestions though.

Good job, great to see how you are going through the whole list of methods!

Awesome, thanks!

Honestly, I don't really see how this is better.
IMO Printer is something that prints, it should be everything that it does. It can accept different parameters tweaking how it prints in its constructor, but if it is a region printer, you should give it a region, and it should print it. It's not a one-use thing.

This is a very complicated patch, I think we'll have to iterate on it quite a lot.
Additionally, we have to be sure that this doesn't crash our performance.

I compared issues produced by this patch to the issues produced before that on all projects from clang/utils/analyzer/projects, and didn't find any difference.

Also, although the test is very extensive, it is pretty lopsided at the same time. C-style cast is only one case out of the myriad of all explicit and, more importantly, implicit casts.

Hey Manas! Great job, you put this together real quick!

Hey, thanks for starting on splitting into more pieces!

This is incredible! Thanks for addressing it! I've encountered this many times.