[PATCH] D86870: [analyzer] Add more tests for ArrayBoundCheckerV2

martong accepted this revision. martong added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/Analysis/out-of-bounds-false-positive.c:34 + +void symbolic_uint_and_int0(unsigned len) { + (void)a[len + 1]; // no-warning ste

[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa787a4ed16d6: [analyzer][StdLibraryFunctionsChecker] Use Optionals throughout the summary API (authored by martong). Changed prior to commit: http

[PATCH] D86870: [analyzer] Add more tests for ArrayBoundCheckerV2

martong added inline comments. Comment at: clang/test/Analysis/out-of-bounds-false-positive.c:34 + +void symbolic_uint_and_int0(unsigned len) { + (void)a[len + 1]; // no-warning steakhal wrote: > martong wrote: > > steakhal wrote: > > > martong wrote: > > > > Hm

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

martong added a comment. I don't have anymore immediate concerns, but I will need more time to comb through the rest of the patch in more details... hopefully I can do that in the following days. Comment at: clang/lib/StaticAnalyzer/Core/ProgramState.cpp:327 +using ObjCForLct

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

martong added inline comments. Comment at: clang/test/Analysis/eval-predefined-exprs.cpp:7-21 + clang_analyzer_dump(__func__); + clang_analyzer_dump(__FUNCTION__); + clang_analyzer_dump(__PRETTY_FUNCTION__); + // expected-warning@-3 {{&Element{"func",0 S64b,char}}} + // expe

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

martong added inline comments. Comment at: clang/test/Analysis/eval-predefined-exprs.cpp:7-21 + clang_analyzer_dump(__func__); + clang_analyzer_dump(__FUNCTION__); + clang_analyzer_dump(__PRETTY_FUNCTION__); + // expected-warning@-3 {{&Element{"func",0 S64b,char}}} + // expe

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

martong created this revision. martong added reviewers: steakhal, balazske, Szelethus, NoQ, vsavchenko. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald ad

[PATCH] D87097: [analyzer][StdLibraryFunctionsChecker] Do not match based on the restrict qualifier in C++

martong created this revision. martong added reviewers: balazske, steakhal, Szelethus, NoQ, vsavchenko, baloghadamsoftware, gamesh411. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, whisperity. Herald ad

[PATCH] D87097: [analyzer][StdLibraryFunctionsChecker] Do not match based on the restrict qualifier in C++

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfe0972d3e4a6: [analyzer][StdLibraryFunctionsChecker] Do not match based on the restrict… (authored by martong). Changed prior to commit: https://r

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

martong added a comment. The key question is this: Why exactly does `1735: ExpectedDecl ImportedOrErr = import(From);` fails to do the import properly when called from ASTImporter::ImportDefinition? Would it be possible to debug that from your LLDB test cases? Maybe starting with the simpler te

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

martong added a comment. I am coping my comments that I've sent in mail, just in case. > However, in the checker, we don't check `byte offset < 0` directly. > We //simplify// the left part of the `byte offset < 0` inequality by > substracting/dividing both sides with the constant `C`, if the `by

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

martong added a comment. About the whole raw offset and the related warning. There is a fundamental question: Should we warn at `&a[0][10]` ? void foo() { int a[10][10]; int *p0 = &a[9][9]; // OK int *p1 = &a[10][10]; // Out-of-bounds static_assert(&a[0][10] == &a[1][0]);

[PATCH] D87138: [analyzer] Introduce refactoring of PthreadLockChecker

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. This seems to be a total non-functional-change. Please include [NFC] next time with a similar refactoring. Otherwise, Looks good to me, thanks! Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D84415: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions

martong updated this revision to Diff 289970. martong added a comment. - Rebase to master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84415/new/ https://reviews.llvm.org/D84415 Files: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecke

[PATCH] D84415: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions

martong added a comment. Polite ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84415/new/ https://reviews.llvm.org/D84415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D84248: [analyzer][StdLibraryFunctionsChecker] Add POSIX time handling functions

martong updated this revision to Diff 289982. martong added a comment. - Rebase to master, ie using optionals everywhere Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84248/new/ https://reviews.llvm.org/D84248 Files: clang/lib/StaticAnalyzer/Che

[PATCH] D84248: [analyzer][StdLibraryFunctionsChecker] Add POSIX time handling functions

martong added a comment. In D84248#2215519 , @Szelethus wrote: > Lets make sure we invite the wider community to see whats going on. > Otherwise, LGTM! I am committing this because it already has two accepts, plus I am //very// confident with the change

[PATCH] D84248: [analyzer][StdLibraryFunctionsChecker] Add POSIX time handling functions

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf0b9dbcfc7ba: [analyzer][StdLibraryFunctionsChecker] Add POSIX time handling functions (authored by martong). Repository: rG LLVM Github Monorepo

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

martong added a comment. In D87081#2256636 , @balazske wrote: > This checker will make an additional assumption on `fread` and `fwrite` with > the ReturnValueCondition. There is nothing new in that. This assumption described by the `.Case` has been here

[PATCH] D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp

martong created this revision. martong added reviewers: Szelethus, balazske, NoQ, vsavchenko. Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald ad

[PATCH] D87240: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies

martong created this revision. martong added reviewers: Szelethus, balazske, NoQ, vsavchenko. Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald ad

[PATCH] D84415: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions

This revision was automatically updated to reflect the committed changes. Closed by commit rGd01280587d97: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions (authored by martong). Changed prior to commit: https://reviews.llvm.org/D84415?vs=289970&id=290296#toc Reposit

[PATCH] D87240: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8248c2af9497: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies (authored by martong). Changed prior to commit: https://reviews

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

martong added a comment. In D87081#2258637 , @Szelethus wrote: > The patch looks great, in fact, it demonstrates how well thought out your > summary crafting machinery is. > > In D87081#2258579 , @martong wrote: >

[PATCH] D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp

martong marked an inline comment as done. martong added a comment. In D87239#2259345 , @steakhal wrote: > I completely agree with you. > I plan to further refactor the CStringChecker, but the related patches are > pretty much stuck :D > > I think this wor

[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

martong added a comment. > But if the string is invalidated (or the new length is completely unknown), > **do not remove** the length from the state; instead, set it to > SymbolConjured. Where exactly do you implement the above? > When strlen(R) is used for the first time on a region R, prod

[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2384-2385 if (SuperRegions.count(MR)) { Entries = F.remove(Entries, MR); + Entries = F.add(Entries, MR, UnknownVal()); continue; martong wrote:

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

martong accepted this revision. martong added a comment. LGTM! Thanks for the clarification and the example you gave. (I agree with @steakhal and I wish if we could get rid of the many lines not-descriptive plist stuff, but that is rather unrelated) CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D76604: [Analyzer] Model `size()` member function of containers

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. This basically looks good to me. Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:482-483 +// of the container (the difference between its `begin()`

[PATCH] D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp

martong added a comment. @steakhal Ping. > I completely agree with you. So, how should we proceed? :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87239/new/ https://reviews.llvm.org/D87239 ___ cfe-co

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

martong added a comment. In D79431#2215610 , @Szelethus wrote: > Ah, okay, silly me. Didn't catch that. Then the message is OK for now. > > edit: Describing //how// the violation happened might be a valuable for > development purposes as well. What if we

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

martong added a comment. In D79431#2263690 , @martong wrote: > In D79431#2215610 , @Szelethus wrote: > >> Ah, okay, silly me. Didn't catch that. Then the message is OK for now. >> >> edit: Describing //how// the vio

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

martong added a comment. In D79431#2265155 , @Szelethus wrote: > In D79431#2263693 , @martong wrote: > >> In D79431#2263690 , @martong wrote: >> >>> What if we'd add a `toStr

[PATCH] D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb7586afc4dcd: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp (authored by martong). Changed prior to commit: https://reviews.llvm.org/D8

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa97648b93846: [analyzer][StdLibraryFunctionsChecker] Add better diagnostics (authored by martong). Changed prior to commit: https://reviews.llvm.o

[PATCH] D92764: [analyzer][StdLibraryFunctionsChecker] Make close and mmap to accept -1 as fd

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd14c63167315: [analyzer][StdLibraryFunctionsChecker] Make close and mmap to accept -1 as fd (authored by martong). Repository: rG LLVM Github Mono

[PATCH] D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints

martong marked 3 inline comments as done. martong added a comment. Thanks for the review! Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1367-1368 +.Case(ReturnsZeroOrMinusOne) .ArgC

[PATCH] D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. martong marked an inline comment as done. Closed by commit rGfebe75032f6f: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints (authored by martong)

[PATCH] D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis.

martong added inline comments. Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:124 /// Note that this class also implements caching. -class CrossTranslationUnitContext { +class CrossTranslationUnitContext : public CrossTUAnalysisHelper { public:

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

martong updated this revision to Diff 310510. martong added a comment. - Remove `if (!DC || !LexicalDC)` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92209/new/ https://reviews.llvm.org/D92209 Files: clang/lib/AST/ASTImporter.cpp clang/unitte

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

martong marked an inline comment as done. martong added a comment. Thanks for the review guys! Comment at: clang/lib/AST/ASTImporter.cpp:2522 + // Add to the lookupTable because we could not do that in MapImported. + Importer.AddToLookupTable(ToTypedef); + te

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

martong updated this revision to Diff 310591. martong marked an inline comment as done. martong added a comment. - Remove not relevant param from test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92209/new/ https://reviews.llvm.org/D92209 Files:

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa5e6590b15b1: [ASTImporter] Support CXXDeductionGuideDecl with local typedef (authored by martong). Changed prior to commit: https://reviews.llvm.

[PATCH] D92962: [ASTImporter] Fix import of a typedef that has an attribute

martong created this revision. martong added a reviewer: teemperor. Herald added subscribers: gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a reviewer: shafik. martong requested review of this revision. Herald added a project: clang. Herald added a subscri

[PATCH] D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes.

martong accepted this revision. martong added a comment. Herald added a subscriber: rnkovacs. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92600/new/ https://reviews.llvm.org/D92600 ___ cfe-commit

[PATCH] D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes.

martong added a comment. I wouldn't worry much about the `ASTMerge` tests, those are mostly legacy and not actively used nowadays. (At one point we were thinking even to remove them because we could got rid of the `ASTMergeAction` which is exclusively used for testing.) The main test infrastruc

[PATCH] D92962: [ASTImporter] Fix import of a typedef that has an attribute

martong updated this revision to Diff 310869. martong added a comment. - Check the attribute in the test as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92962/new/ https://reviews.llvm.org/D92962 Files: clang/lib/AST/ASTImporter.cpp clan

[PATCH] D92962: [ASTImporter] Fix import of a typedef that has an attribute

martong marked an inline comment as done. martong added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:6098 + FirstDeclMatcher().match(TU, typedefDecl(hasName("X"))); + auto *ToD = Import(FromD, Lang_CXX17); + ASSERT_TRUE(ToD); shafik

[PATCH] D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes.

martong accepted this revision. martong added a comment. Thanks for the update. I checked it, still looks good to me. Some notes in parenthesis: > It looks like the problem is again due to delayed template parsing: the > templatized functions in the test both came out as nullptr. So, I added a

[PATCH] D92962: [ASTImporter] Fix import of a typedef that has an attribute

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. martong marked an inline comment as done. Closed by commit rG68f53960e17d: [ASTImporter] Fix import of a typedef that has an attribute (authored by martong). Repositor

[PATCH] D92962: [ASTImporter] Fix import of a typedef that has an attribute

martong added a comment. Shafik, thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92962/new/ https://reviews.llvm.org/D92962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

martong added a comment. In D92634#2451646 , @danielmarjamaki wrote: > No reviews => I will not contribute. Hi, thanks for the patch! I am terribly sorry that we could not pick this up earlier. You can always ping the community with a simple 'Ping' mess

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

martong added a comment. In D92634#2453822 , @OikawaKirie wrote: > I think it could be better to implement this check with a checker on > `PreStmt` and so on. And IMO, checkers have enough > functionalities to report these problems. > > Besides, the retu

[PATCH] D92797: APINotes: add initial stub of APINotesWriter

martong added a comment. In D92797#2452497 , @compnerd wrote: > Ping I am terribly sorry for keeping you waiting always. Thanks for your patience. Comment at: clang/include/clang/APINotes/APINotesWriter.h:1 +//===-- APINotesWriter.h -

[PATCH] D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes.

martong requested changes to this revision. martong added inline comments. This revision now requires changes to proceed. Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1656-1662 + R"( + template + void f() { +T x; +(void)_Generic(x,

[PATCH] D93222: [RFC][analyzer] Introduce MacroExpansionContext to libAnalysis

martong added a comment. Nice and precise work! And special thanks for the unit tests. I've found some minor nits. Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:77 + /// \param MacroExpansionLoc Must be the expansion location of a macro. + /// \return The

[PATCH] D93223: [RFC][analyzer] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers

martong added a comment. Looks quite straight-forward other than that new prototype. Comment at: clang/include/clang/Analysis/PathDiagnostic.h:911 +void createHTMLSingleFileDiagnosticConsumer( +PathDiagnosticConsumerOptions DiagOpts, Why do we need this p

[PATCH] D93222: [RFC][analyzer] Introduce MacroExpansionContext to libAnalysis

martong added inline comments. Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:82 + MacroExpansionText + getExpandedMacroForLocation(SourceLocation MacroExpansionLoc) const; + `getExpandedText` ? Since what you get back is not a macro anymore.

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

martong added inline comments. Comment at: clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:148 nameSET_PTR_VAR_TO_NULL - expansionptr = 0 + expansionptr =0 I wonder how much added value do we have with these h

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

martong added inline comments. Comment at: clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:148 nameSET_PTR_VAR_TO_NULL - expansionptr = 0 + expansionptr =0 martong wrote: > I wonder how much added value do we

[PATCH] D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes.

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Thanks for your work, I am glad that finally the Window bug is fixed with a simple solution. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D926

[PATCH] D91104: APINotes: add property models for YAML attributes

martong added inline comments. Comment at: clang/include/clang/APINotes/Types.h:60 + /// Whether SwiftPrivate was specified. + unsigned SwiftPrivateSpecified : 1; + I was wondering whether Swift specific properties are meaningful to all descendants of `CommonE

[PATCH] D91104: APINotes: add property models for YAML attributes

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Thanks, looks good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91104/new/ https://reviews.llvm.org/D91104 __

[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

martong added a comment. > In this example, it cast to the `unsigned char` (which is the type of the > stored value of `**b`, stored at `#1`) instead of the static type of the > access (the type of `**b` is `char`at `#2`). Possible typo in the summary. At `#2` the type should be `char *` shoul

[PATCH] D88019: [analyzer][solver] Fix issue with symbol non-equality tracking

martong added a comment. In D88019#2296337 , @steakhal wrote: > In D88019#2291953 , @steakhal wrote: > >> What are our options mitigating anything similar happening in the future? >> >> This way any change touching

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

martong created this revision. martong added reviewers: balazske, teemperor, shafik, a_sidorin. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a project: clang. martong requested review of this revision. Repository:

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

martong added a comment. The added test causes the below assertion in the baseline (w/o the fix): ASTTests: ../../git/llvm-project/clang/lib/AST/ExprConstant.cpp:14543: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext&, llvm::SmallVectorImpl >*) const: Assertion `!isVal

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

martong added a comment. > In D88665#2307562 , @shafik wrote: > >> So was the bug we were saying there were falsely equivalent or falsely not >> equivalent? > > I think the bug is the crash :) Yes. More specifically, the call of `getBitWidthValue()` cause

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

martong updated this revision to Diff 295800. martong added a comment. - Delegate to BitWidth Expr matching in case of BitFields Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88665/new/ https://reviews.llvm.org/D88665 Files: clang/lib/AST/ASTStr

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

martong requested changes to this revision. martong added a comment. This revision now requires changes to proceed. Sorry for the late review, I just noticed something which is not a logical error, but we could make the ASTImporter code much cleaner. Comment at: clang/lib/AST/

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:9010 + ToPath[Idx] = + cast(const_cast(ImpDecl.get())); +} Tyker wrote: > rsmith wrote: > > We want the path in an `APValue` to be canonical, but importing a canonical > >

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG007dd12d5468: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl (authored by martong). Repository: rG LLVM Github Monorep

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

martong added a comment. > Currently, we are totally inconsistent about the diagnostics. Either we > should emit a diagnostic before all return false or we should not ever emit > any diags. The diagnostics in their current form are misleading, because > there could be many notes missing. I am n

[PATCH] D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex

martong created this revision. martong added reviewers: teemperor, shafik. Herald added subscribers: cfe-commits, pengfei, gamesh411, Szelethus, arphaman, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a project: clang. martong requested review of this revision. During the imp

[PATCH] D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr

martong created this revision. martong added reviewers: teemperor, shafik. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a project: clang. martong requested review of this revision. During the import of FormatAttrs w

[PATCH] D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr

martong added a comment. Thanks for the review! Comment at: clang/test/ASTMerge/attr/Inputs/FormatAttr.cpp:2 +int foo(const char * fmt, ...) +__attribute__ ((__format__ (__scanf__, 1, 2))); teemperor wrote: > (Not sure if we care about that in tests, but that's

[PATCH] D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. martong marked an inline comment as done. Closed by commit rGdd965711c9f0: [ASTImporter] Fix crash caused by unimported type of FromatAttr (authored by martong). Chang

[PATCH] D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex

martong marked an inline comment as done. martong added a comment. Thanks for the reivew! Comment at: clang/lib/AST/ASTImporter.cpp:8109 + FromAttr->getAttributeSpellingListIndex()); + ToAttr->setPackExpansion(FromAttr->isPackExpansion()); + ToAttr->setImplicit(FromAttr-

[PATCH] D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. martong marked an inline comment as done. Closed by commit rG73c6beb2f705: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex (authored by martong). Ch

[PATCH] D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex

martong added a comment. Ups, sorry for the break and thanks for the notice! I am going to investigate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89318/new/ https://reviews.llvm.org/D89318 ___ cfe-co

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

martong added a comment. In D63640#2331410 , @rsmith wrote: > Reverse ping: I have a patch implementing class type non-type template > parameters that's blocked on this landing. If you won't have time to address > @martong's comments soon, do you mind if

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

martong added a comment. In D63640#2331779 , @Tyker wrote: > In D63640#2331734 , @martong wrote: > >> In D63640#2331410 , @rsmith wrote: >> >>> Reverse ping: I have a patch i

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added a comment. Seems like this patch handles already existing attributes in Clang, so this might not be affected by the concerns brought up here by James: http://lists.llvm.org/pipermail/cfe-dev/2020-October/066996.html Comment at: clang/include/clang/APINotes/APINot

[PATCH] D89528: [clang][test] Fix prefix operator++ signature in iterators

martong added a comment. What is the context here? Did it cause any crash/bug or were you just reading through the code for a good night sleep? :D Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89528/new/ https://reviews.llvm.org/D89528 __

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

martong created this revision. martong added a reviewer: rsmith. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: shafik. Herald added a project: clang. martong requested review of this revision. http://lists.llvm.org/pipermail/cfe-dev/2020-No

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

martong added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:5893 + // in the global scope. + EXPECT_EQ(Param->getType()->getAs()->getDecl(), Typedef); +} Note, this "expectation" fails in baseline. Repository: rG LLVM Github Monorepo

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

martong created this revision. martong added reviewers: teemperor, shafik, balazske. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a project: clang. martong requested review of this revision. CXXDeductionGuideDecl is

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

martong updated this revision to Diff 307848. martong added a comment. - Add test for user-defined ctad - Handle IsCopyDeductionCandidate - Fix typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92109/new/ https://reviews.llvm.org/D92109 Files:

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

martong added a comment. Thanks for the review! Your comment made me think about whether we handle the deduced template class properly (`getDeducedTemplate`). Then I realized we do import that when we import the `DeclarationName`. Anyway I added a check for that in the test. Repository: rG

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

martong created this revision. martong added reviewers: teemperor, balazske. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a reviewer: shafik. Herald added a project: clang. martong requested review of this revision.

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

martong updated this revision to Diff 307988. martong marked 2 inline comments as done. martong added a comment. - Update test to match the copy deduction guide Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92109/new/ https://reviews.llvm.org/D9210

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

martong added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:5900 + ASSERT_TRUE(ToD); + EXPECT_EQ(FromD->isCopyDeductionCandidate(), ToD->isCopyDeductionCandidate()); + // Check that the deduced class template is also imported. teemperor

[PATCH] D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64

martong added a comment. In D92307#2422468 , @vsavchenko wrote: > That's a good fix! > How did this happen though that max value of `off_t` was even used for `fd`. > Seems pretty odd! I used a semi-automated approach to create these summaries from cppch

[PATCH] D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92307/new/ https://reviews.llvm.org/D92307

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

martong updated this revision to Diff 308335. martong marked 2 inline comments as done. martong added a comment. - Check for dependent DeclContext - Remove "dump()" calls from tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92101/new/ https://r

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

martong added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:2356-2359 + auto *CD = cast(OldParam->getDeclContext()); + // If the typedef is not a local typedef, then skip the transform. + if (OldTypedefDecl->getDeclContext() != CD->getDeclContext())

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG70eb2ce395be: [ASTImporter] Support import of CXXDeductionGuideDecl (authored by martong). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3336 +if (Fun->getExplicitSpecifier().getExpr()) { + ExplicitExpr = importChecked(Err, Fun->getExplicitSpecifier().getExpr()); + if (Err) rnk wrote: > GCC 5.3 complained about

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

martong added a comment. In D92101#2423685 , @shafik wrote: > Should we add a test for the `regular function template` case as well? I think this issue is specific for `CXXDeductionGuideDecl`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

martong updated this revision to Diff 308920. martong added a comment. - Move dependent context check into TransformTypedefType - Add new test case for param with pointer type Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92101/new/ https://reviews

<    9   10   11   12   13   14   15   16   17   18   >