Static analyzer cherrypicks 25 #3982
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No behavior change. (cherry picked from commit 0b7c9ad)
@finally is still not implemented. With this, clang can emit -Wreturn-type warnings for functions containing @try/@catch (but not yet @finally), and -Wunreachable-code also works for those functions. The implementation is similar to D36914. Part of PR46693. Differential Revision: https://reviews.llvm.org/D112287 (cherry picked from commit 04f3079)
@finally is still not implemented. With this, clang can emit -Wreturn-type warnings for functions containing @try/@catch (but not yet @finally), and -Wunreachable-code also works for those functions. The implementation is similar to D36914. Part of PR46693. Differential Revision: https://reviews.llvm.org/D112287 (cherry picked from commit 04f3079)
It seems like protobuf crashed the `std::string` checker. Somehow it acquired `UnknownVal` as the sole `std::string` constructor parameter, causing a crash in the `castAs<Loc>()`. This patch addresses this. Reviewed By: martong Differential Revision: https://reviews.llvm.org/D112551 (cherry picked from commit c184072)
Initiate the reorganization of the equality information during symbol simplification. E.g., if we bump into `c + 1 == 0` during simplification then we'd like to express that `c == -1`. It makes sense to do this only with `SymIntExpr`s. Reviewed By: steakhal Differential Revision: https://reviews.llvm.org/D111642 (cherry picked from commit 888af47)
We can reuse the "adjustment" handling logic in the higher level of the solver by calling `State->assume`. Differential Revision: https://reviews.llvm.org/D112296 (cherry picked from commit a8297ed)
Due to a typo, `sprintf()` was recognized as a taint source instead of a taint propagator. It was because an empty taint source list - which is the first parameter of the `TaintPropagationRule` - encoded the unconditional taint sources. This typo effectively turned the `sprintf()` into an unconditional taint source. This patch fixes that typo and demonstrated the correct behavior with tests. Reviewed By: martong Differential Revision: https://reviews.llvm.org/D112558 (cherry picked from commit 49285f4)
…for constant arrays. Summary: Assuming that values of constant arrays never change, we can retrieve values for specific position(index) right from the initializer, if presented. Retrieve a character code by index from StringLiteral which is an initializer of constant arrays in global scope. This patch has a known issue of getting access to characters past the end of the literal. The declaration, in which the literal is used, is an implicit cast of kind `array-to-pointer`. The offset should be in literal length's bounds. This should be distinguished from the states in the Standard C++20 [dcl.init.string] 9.4.2.3. Example: const char arr[42] = "123"; char c = arr[41]; // OK const char * const str = "123"; char c = str[41]; // NOK Differential Revision: https://reviews.llvm.org/D107339 (cherry picked from commit 1deccd0)
We only need the operating system name, not all information. Differential Revision: https://reviews.llvm.org/D111797 (cherry picked from commit 6ecd4a4)
Previously, if accidentally multiple checkers `eval::Call`-ed the same `CallEvent`, in debug builds the analyzer detected this and crashed with the message stating this. Unfortunately, the message did not state the offending checkers violating this invariant. This revision addresses this by printing a more descriptive message before aborting. Reviewed By: martong Differential Revision: https://reviews.llvm.org/D112889 (cherry picked from commit 9b5c9c4)
(cherry picked from commit eb1c7c1)
…onal array declaration. Summary: Add support of multi-dimensional arrays in `RegionStoreManager::getBindingForElement`. Handle nested ElementRegion's getting offsets and checking for being in bounds. Get values from the nested initialization lists using obtained offsets. Differential Revision: https://reviews.llvm.org/D111654 (cherry picked from commit a12bfac)
… with constants
D103314 introduced symbol simplification when a new constant constraint is
added. Currently, we simplify existing equivalence classes by iterating over
all existing members of them and trying to simplify each member symbol with
simplifySVal.
At the end of such a simplification round we may end up introducing a
new constant constraint. Example:
```
if (a + b + c != d)
return;
if (c + b != 0)
return;
// Simplification starts here.
if (b != 0)
return;
```
The `c == 0` constraint is the result of the first simplification iteration.
However, we could do another round of simplification to reach the conclusion
that `a == d`. Generally, we could do as many new iterations until we reach a
fixpoint.
We can reach to a fixpoint by recursively calling `State->assume` on the
newly simplified symbol. By calling `State->assume` we re-ignite the
whole assume machinery (along e.g with adjustment handling).
Why should we do this? By reaching a fixpoint in simplification we are capable
of discovering infeasible states at the moment of the introduction of the
**first** constant constraint.
Let's modify the previous example just a bit, and consider what happens without
the fixpoint iteration.
```
if (a + b + c != d)
return;
if (c + b != 0)
return;
// Adding a new constraint.
if (a == d)
return;
// This brings in a contradiction.
if (b != 0)
return;
clang_analyzer_warnIfReached(); // This produces a warning.
// The path is already infeasible...
if (c == 0) // ...but we realize that only when we evaluate `c == 0`.
return;
```
What happens currently, without the fixpoint iteration? As the inline comments
suggest, without the fixpoint iteration we are doomed to realize that we are on
an infeasible path only after we are already walking on that. With fixpoint
iteration we can detect that before stepping on that. With fixpoint iteration,
the `clang_analyzer_warnIfReached` does not warn in the above example b/c
during the evaluation of `b == 0` we realize the contradiction. The engine and
the checkers do rely on that either `assume(Cond)` or `assume(!Cond)` should be
feasible. This is in fact assured by the so called expensive checks
(LLVM_ENABLE_EXPENSIVE_CHECKS). The StdLibraryFuncionsChecker is notably one of
the checkers that has a very similar assertion.
Before this patch, we simply added the simplified symbol to the equivalence
class. In this patch, after we have added the simplified symbol, we remove the
old (more complex) symbol from the members of the equivalence class
(`ClassMembers`). Removing the old symbol is beneficial because during the next
iteration of the simplification we don't have to consider again the old symbol.
Contrary to how we handle `ClassMembers`, we don't remove the old Sym->Class
relation from the `ClassMap`. This is important for two reasons: The
constraints of the old symbol can still be found via it's equivalence class
that it used to be the member of (1). We can spare one removal and thus one
additional tree in the forest of `ClassMap` (2).
Performance and complexity: Let us assume that in a State we have N non-trivial
equivalence classes and that all constraints and disequality info is related to
non-trivial classes. In the worst case, we can simplify only one symbol of one
class in each iteration. The number of symbols in one class cannot grow b/c we
replace the old symbol with the simplified one. Also, the number of the
equivalence classes can decrease only, b/c the algorithm does a merge operation
optionally. We need N iterations in this case to reach the fixpoint. Thus, the
steps needed to be done in the worst case is proportional to `N*N`. Empirical
results (attached) show that there is some hardly noticeable run-time and peak
memory discrepancy compared to the baseline. In my opinion, these differences
could be the result of measurement error.
This worst case scenario can be extended to that cases when we have trivial
classes in the constraints and in the disequality map are transforming to such
a State where there are only non-trivial classes, b/c the algorithm does merge
operations. A merge operation on two trivial classes results in one non-trivial
class.
Differential Revision: https://reviews.llvm.org/D106823
(cherry picked from commit 806329d)
We no longer need a reference to RangedConstraintManager, we call top level `State->assume` functions. Differential Revision: https://reviews.llvm.org/D113261 (cherry picked from commit 01c9700)
…ifiers. Summary: Specifically, this fixes the case when we get an access to array element through the pointer to element. This covers several FIXME's. in https://reviews.llvm.org/D111654. Example: const int arr[4][2]; const int *ptr = arr[1]; // Fixes this. The issue is that `arr[1]` is `int*` (&Element{Element{glob_arr5,1 S64b,int[2]},0 S64b,int}), and `ptr` is `const int*`. We don't take qualifiers into account. Consequently, we doesn't match the types as the same ones. Differential Revision: https://reviews.llvm.org/D113480 (cherry picked from commit f0bc7d2)
`CallDescriptions` deserve its own translation unit. This patch simply moves the corresponding parts. Also includes the `CallDescription.h` where it's necessary. Reviewed By: martong, xazax.hun, Szelethus Differential Revision: https://reviews.llvm.org/D113587 (cherry picked from commit 0b9d3a6)
The new //deleted// constructor overload makes sure that no implicit conversion from `0` would happen to `ArrayRef<const char*>`. Also adds nodiscard to the `CallDescriptionMap::lookup()` (cherry picked from commit 35ff3a0)
Sometimes we only want to decide if some function is called, and we don't care which of the set. This `CallDescriptionSet` will have the same behavior, except instead of `lookup()` returning a pointer to the mapped value, the `contains()` returns `bool`. Internally, it uses the `CallDescriptionMap<bool>` for implementing the behavior. It is preferred, to reuse the generic `CallDescriptionMap::lookup()` logic, instead of duplicating it. The generic version might be improved by implementing a hash lookup or something along those lines. Reviewed By: martong, Szelethus Differential Revision: https://reviews.llvm.org/D113589 (cherry picked from commit d448fcd)
…sCalled() This patch introduces `CallDescription::matches()` member function, accepting a `CallEvent`. Semantically, `Call.isCalled(CD)` is the same as `CD.matches(Call)`. The patch also introduces the `matchesAny()` variadic free function template. It accepts a `CallEvent` and at least one `CallDescription` to match against. Reviewed By: martong Differential Revision: https://reviews.llvm.org/D113590 (cherry picked from commit 6c51270)
… isCalled() This patch replaces each use of the previous API with the new one. In variadic cases, it will use the ADL `matchesAny(Call, CDs...)` variadic function. Also simplifies some code involving such operations. Reviewed By: martong, xazax.hun Differential Revision: https://reviews.llvm.org/D113591 (cherry picked from commit f18da19)
Reviewed By: martong, xazax.hun Differential Revision: https://reviews.llvm.org/D113592 (cherry picked from commit 9ad0a90)
Previously, CallDescription simply referred to the qualified name parts by `const char*` pointers. In the future we might want to dynamically load and populate `CallDescriptionMaps`, hence we will need the `CallDescriptions` to actually **own** their qualified name parts. Reviewed By: martong, xazax.hun Differential Revision: https://reviews.llvm.org/D113593 (cherry picked from commit de9d7e4)
`CallDescriptions` have a `RequiredArgs` and `RequiredParams` members, but they are of different types, `unsigned` and `size_t` respectively. In the patch I use only `unsigned` for both, that should be large enough anyway. I also introduce the `MaybeUInt` type alias for `Optional<unsigned>`. Additionally, I also avoid the use of the //smart// less-than operator. template <typename T> constexpr bool operator<=(const Optional<T> &X, const T &Y); Which would check if the optional **has** a value and compare the data only after. I found it surprising, thus I think we are better off without it. Reviewed By: martong, xazax.hun Differential Revision: https://reviews.llvm.org/D113594 (cherry picked from commit 97f1bf1)
Yeah, let's prefer a slightly stronger type representing this. Reviewed By: martong, xazax.hun Differential Revision: https://reviews.llvm.org/D113595 (cherry picked from commit e6ef134)
I forgot to include this in D113594 Differential Revision: https://reviews.llvm.org/D113594 (cherry picked from commit d5de568)
Make the SimpleSValBuilder capable to simplify existing IntSym expressions based on a newly added constraint on the sub-expression. Differential Revision: https://reviews.llvm.org/D113754 (cherry picked from commit ffc32ef)
Make the SValBuilder capable to simplify existing SVals based on a newly added constraints when evaluating a BinOp. Before this patch, we called `simplify` only in some edge cases. However, we can and should investigate the constraints in all cases. Differential Revision: https://reviews.llvm.org/D113753 (cherry picked from commit 12887a2)
I just read this part of the code, and I found the nested ifs less readable. Reviewed By: martong Differential Revision: https://reviews.llvm.org/D114441 (cherry picked from commit af37d4b)
Currently, during symbol simplification we remove the original member symbol from the equivalence class (`ClassMembers` trait). However, we keep the reverse link (`ClassMap` trait), in order to be able the query the related constraints even for the old member. This asymmetry can lead to a problem when we merge equivalence classes: ``` ClassA: [a, b] // ClassMembers trait, a->a, b->a // ClassMap trait, a is the representative symbol ``` Now lets delete `a`: ``` ClassA: [b] a->a, b->a ``` Let's merge the trivial class `c` into ClassA: ``` ClassA: [c, b] c->c, b->c, a->a ``` Now after the merge operation, `c` and `a` are actually in different equivalence classes, which is inconsistent. One solution to this problem is to simply avoid removing the original member and this is what this patch does. Other options I have considered: 1) Always merge the trivial class into the non-trivial class. This might work most of the time, however, will fail if we have to merge two non-trivial classes (in that case we no longer can track equivalences precisely). 2) In `removeMember`, update the reverse link as well. This would cease the inconsistency, but we'd loose precision since we could not query the constraints for the removed member. Differential Revision: https://reviews.llvm.org/D114619 (cherry picked from commit f02c5f3)
…bols in the tree Add the capability to simplify more complex constraints where there are 3 symbols in the tree. In this change I extend simplifySVal to query constraints of children sub-symbols in a symbol tree. (The constraint for the parent is asked in getKnownValue.) Differential Revision: https://reviews.llvm.org/D103317 (cherry picked from commit 0a17896)
… eq class merge This reverts commit f02c5f3 and addresses the issue mentioned in D114619 differently. Repeating the issue here: Currently, during symbol simplification we remove the original member symbol from the equivalence class (`ClassMembers` trait). However, we keep the reverse link (`ClassMap` trait), in order to be able the query the related constraints even for the old member. This asymmetry can lead to a problem when we merge equivalence classes: ``` ClassA: [a, b] // ClassMembers trait, a->a, b->a // ClassMap trait, a is the representative symbol ``` Now let,s delete `a`: ``` ClassA: [b] a->a, b->a ``` Let's merge ClassA into the trivial class `c`: ``` ClassA: [c, b] c->c, b->c, a->a ``` Now, after the merge operation, `c` and `a` are actually in different equivalence classes, which is inconsistent. This issue manifests in a test case (added in D103317): ``` void recurring_symbol(int b) { if (b * b != b) if ((b * b) * b * b != (b * b) * b) if (b * b == 1) } ``` Before the simplification we have these equivalence classes: ``` trivial EQ1: [b * b != b] trivial EQ2: [(b * b) * b * b != (b * b) * b] ``` During the simplification with `b * b == 1`, EQ1 is merged with `1 != b` `EQ1: [b * b != b, 1 != b]` and we remove the complex symbol, so `EQ1: [1 != b]` Then we start to simplify the only symbol in EQ2: `(b * b) * b * b != (b * b) * b --> 1 * b * b != 1 * b --> b * b != b` But `b * b != b` is such a symbol that had been removed previously from EQ1, thus we reach the above mentioned inconsistency. This patch addresses the issue by making it impossible to synthesise a symbol that had been simplified before. We achieve this by simplifying the given symbol to the absolute simplest form. Differential Revision: https://reviews.llvm.org/D114887 (cherry picked from commit 20f8733)
Some projects [1,2,3] have flex-generated files besides bison-generated ones. Unfortunately, the comment `"/* A lexical scanner generated by flex */"` generated by the tools is not necessarily at the beginning of the file, thus we need to quickly skim through the file for this needle string. Luckily, StringRef can do this operation in an efficient way. That being said, now the bison comment is not required to be at the very beginning of the file. This allows us to detect a couple more cases [4,5,6]. Alternatively, we could say that we only allow whitespace characters before matching the bison/flex header comment. That would prevent the (probably) unnecessary string search in the buffer. However, I could not verify that these tools would actually respect this assumption. Additionally to this, e.g. the Twin project [1] has other non-whitespace characters (some preprocessor directives) before the flex-generated header comment. So the heuristic in the previous paragraph won't work with that. Thus, I would advocate the current implementation. According to my measurement, this patch won't introduce measurable performance degradation, even though we will do 2 linear scans. I introduce the ignore-bison-generated-files and ignore-flex-generated-files to disable skipping these files. Both of these options are true by default. [1]: https://github.com/cosmos72/twin/blob/master/server/rcparse_lex.cpp#L7 [2]: https://github.com/marcauresoar/make-examples/blob/22362cdcf9dd7c597b5049ce7f176621e2e9ac7a/sandbox/count-words/lexer.c#L6 [3]: https://github.com/vladcmanea/2nd-faculty-year-Formal-Languages---Automata-assignments/blob/11abdf64629d9eb741438ba69f04636769d5a374/lab1/lex.yy.c#L6 [4]: https://github.com/KritikaChoudhary/System-Software-Lab/blob/47f5b2cfe2a2738fd54eae9f8439817f6a22034e/B_yacc/1/y1.tab.h#L2 [5]: https://github.com/VirtualMonitor/VirtualMonitor/blob/71d1bf9b1e7b392a7bd0c73dc217138dc5865651/src/VBox/Additions/x11/x11include/xorg-server-1.8.0/parser.h#L2 [6]: https://github.com/bspaulding/DrawTest/blob/3f773ceb13de14275429036b9cbc5aa19e29bab9/Framework/OpenEars.framework/Versions/A/Headers/jsgf_parser.h#L2 Reviewed By: xazax.hun Differential Revision: https://reviews.llvm.org/D114510 (cherry picked from commit 9873ef4)
Previously, the `SValBuilder` could not encounter expressions of the following kind: NonLoc OP Loc Loc OP NonLoc Where the `Op` is other than `BO_Add`. As of now, due to the smarter simplification and the fixedpoint iteration, it turns out we can. It can happen if the `Loc` was perfectly constrained to a concrete value (`nonloc::ConcreteInt`), thus the simplifier can do constant-folding in these cases as well. Unfortunately, this could cause assertion failures, since we assumed that the operator must be `BO_Add`, causing a crash. --- In the patch, I decided to preserve the original behavior (aka. swap the operands (if the operator is commutative), but if the `RHS` was a `loc::ConcreteInt` call `evalBinOpNN()`. I think this interpretation of the arithmetic expression is closer to reality. I also tried naively introducing a separate handler for `loc::ConcreteInt` RHS, before doing handling the more generic `Loc` RHS case. However, it broke the `zoo1backwards()` test in the `nullptr.cpp` file. This highlighted for me the importance to preserve the original behavior for the `BO_Add` at least. PS: Sorry for introducing yet another branch into this `evalBinOpXX` madness. I've got a couple of ideas about refactoring these. We'll see if I can get to it. The test file demonstrates the issue and makes sure nothing similar happens. The `no-crash` annotated lines show, where we crashed before applying this patch. Reviewed By: martong Differential Revision: https://reviews.llvm.org/D115149 (cherry picked from commit a6816b9)
Move the SymExpr simplification fixpoint logic into SValBuilder. Differential Revision: https://reviews.llvm.org/D114938 (cherry picked from commit 978431e)
A series of unary operators and casts may obscure the variable we're trying to analyze. Ignore them for the uninitialized value analysis. Other checks determine if the unary operators result in a valid l-value. Link: ClangBuiltLinux/linux#1521 Reviewed By: nickdesaulniers Differential Revision: https://reviews.llvm.org/D114848 (cherry picked from commit c4582a6)
This avoids an unnecessary copy required by 'return OS.str()', allowing instead for NRVO or implicit move. The .str() call (which flushes the stream) is no longer required since 65b1361, which made raw_string_ostream unbuffered by default. Differential Revision: https://reviews.llvm.org/D115374 (cherry picked from commit 715c72b)
This avoids an unnecessary copy required by 'return OS.str()', allowing instead for NRVO or implicit move. The .str() call (which flushes the stream) is no longer required since 65b1361, which made raw_string_ostream unbuffered by default. Differential Revision: https://reviews.llvm.org/D115374 (cherry picked from commit 08eb614)
…tersections and adjacency Summary: Handle intersected and adjacent ranges uniting them into a single one. Example: intersection [0, 10] U [5, 20] = [0, 20] adjacency [0, 10] U [11, 20] = [0, 20] Differential Revision: https://reviews.llvm.org/D99797 (cherry picked from commit 6a399bf)
…flow and underflow This expands checking for more expressions. This will check underflow and loss of precision when using call expressions like: void foo(unsigned); int i = -1; foo(i); This also includes other expressions as well, so it can catch negative indices to std::vector since it uses unsigned integers for [] and .at() function. Patch by: @pfultz2 Differential Revision: https://reviews.llvm.org/D46081 (cherry picked from commit bd9e239)
…function from SVal to Optional<SVal> Summary: Refactor return value of `StoreManager::attemptDownCast` function by removing the last parameter `bool &Failed` and replace the return value `SVal` with `Optional<SVal>`. Make the function consistent with the family of `evalDerivedToBase` by renaming it to `evalBaseToDerived`. Aligned the code on the call side with these changes. Differential Revision: https://reviews.llvm.org/ (cherry picked from commit da8bd97)
CallDescriptionMap is supposed to be immutable and opaque about the stored CallDescriptions, but moving a CallDescriptionMap does not violate these principles. Reviewed By: steakhal Differential Revision: https://reviews.llvm.org/D115931 (cherry picked from commit e0321eb)
CallDescriptionMap benefits from a range constructor when the CallDescription and mapped type pairs cannot be constructed at once, but are built incrementally. Reviewed By: steakhal Differential Revision: https://reviews.llvm.org/D115934 (cherry picked from commit c6a861e)
An identical declaration is present just a couple of lines above the line being removed in this patch. Identified with readability-redundant-declaration. (cherry picked from commit 34558b0)
When performing CFG based analyses, don't forget to check the child statements of an asm goto, such as the expressions used for inputs+outputs. Fixes: llvm#51024 Fixes: ClangBuiltLinux/linux#1439 Reviewed By: void, jyknight, jyu2, efriedma Differential Revision: https://reviews.llvm.org/D116059 (cherry picked from commit 3a604fd)
…er::evalCast Summary: Produce SymbolCast for integral types in `evalCast` function. Apply several simplification techniques while producing the symbols. Added a boolean option `handle-integral-cast-for-ranges` under `-analyzer-config` flag. Disabled the feature by default. Differential Revision: https://reviews.llvm.org/D105340 (cherry picked from commit d835dd4)
GenericTaintChecker now uses CallDescriptionMap to describe the possible operation in code which trigger the introduction (sources), the removal (filters), the passing along (propagations) and detection (sinks) of tainted values. Reviewed By: steakhal, NoQ Differential Revision: https://reviews.llvm.org/D116025 (cherry picked from commit 17f7424)
Differential Revision: https://reviews.llvm.org/D117568 (cherry picked from commit 280b430)
`CallDescriptions` for builtin functions relaxes the match rules
somewhat, so that the `CallDescription` will match for calls that have
some prefix or suffix. This was achieved by doing a `StringRef::contains()`.
However, this is somewhat problematic for builtins that are substrings
of each other.
Consider the following:
`CallDescription{ builtin, "memcpy"}` will match for
`__builtin_wmemcpy()` calls, which is unfortunate.
This patch addresses/works around the issue by checking if the
characters around the function's name are not part of the 'name'
semantically. In other words, to accept a match for `"memcpy"` the call
should not have alphanumeric (`[a-zA-Z]`) characters around the 'match'.
So, `CallDescription{ builtin, "memcpy"}` will not match on:
- `__builtin_wmemcpy: there is a `w` alphanumeric character before the match.
- `__builtin_memcpyFOoBar_inline`: there is a `F` character after the match.
- `__builtin_memcpyX_inline`: there is an `X` character after the match.
But it will still match for:
- `memcpy`: exact match
- `__builtin_memcpy`: there is an _ before the match
- `__builtin_memcpy_inline`: there is an _ after the match
- `memcpy_inline_builtinFooBar`: there is an _ after the match
Reviewed By: NoQ
Differential Revision: https://reviews.llvm.org/D118388
(cherry picked from commit abc8736)
There is different bug types for different types of bugs but the **emitAdditionOverflowbug** seems to use bugtype **BT_NotCSting** but actually it have to use **BT_AdditionOverflow** . Reviewed By: steakhal Differential Revision: https://reviews.llvm.org/D119462 (cherry picked from commit 6745b6a)
… frames. LocationContext::getDecl() isn't useful for obtaining the "farmed" body because the (synthetic) body statement isn't actually attached to the (natural-grown) declaration in the AST. Differential Revision: https://reviews.llvm.org/D119509 (cherry picked from commit e0e1748)
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
t-rasmud
approved these changes
Feb 22, 2022
Yeah this happens from time to time. 989e905 is accepted by Gabor but previously marked as rejected by Valeriy with minor comments; Valeriy isn't working on the static analyzer anymore so he couldn't update his verdict as these comments were fixed but phabricator still thinks it's 1:1 so it's not marking the whole revision as accepted. bd4f579 is a relatively small patch by Gabor who already contributed a lot to our ad-hoc constraint solver. I think post-commit review (with the verbal accept by Balázs) is appropriate here. To be fair, I also think I should have at least said "cool!" because the patch is really cool :)
Ok jenkins crashed, third time the charm I guess. |
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clang Static Analyzer is traditionally kept reasonably fresh on stable branches through continuous cherry-picking.