[PATCH] D56708: NFC: Implement OMPClause dump in terms of visitors
steveire marked an inline comment as done.
steveire added inline comments.
Comment at: lib/AST/TextNodeDumper.cpp:276-280
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
aaron.ballman wrote:
> This pattern is coming up quite frequently. I wonder if we should factor this
> out into something like:
> ```
> template
> bool dumpNullObject(const Ty *Val, StringRef Label) const {
> if (!Val) {
> ColorScope Color(OS, ShowColors, NullColor);
> OS << "<<>> " << Label;
> }
> return !Val;
> }
> ```
> So that uses become:
> ```
> if (dumpNullObject(Whatever, "Whatever"))
> return;
> ```
> Doesn't have to be done as part of this patch, but it seems like it might
> reduce some redundancy.
Something to come back to later I think. It would be more consistent now to
have the label on the left side (dumping the various parts of an if() which may
be nullptr), and in this OMPClause case, to not have a label on the right side.
Also - at least the 'OS << "<<>>"' in the comment visitor is dead
unreachable code.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56708/new/
https://reviews.llvm.org/D56708
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56708: NFC: Implement OMPClause dump in terms of visitors
This revision was automatically updated to reflect the committed changes.
Closed by commit rL351236: NFC: Implement OMPClause dump in terms of visitors
(authored by steveire, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D56708?vs=181749=181856#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56708/new/
https://reviews.llvm.org/D56708
Files:
cfe/trunk/include/clang/AST/TextNodeDumper.h
cfe/trunk/lib/AST/ASTDumper.cpp
cfe/trunk/lib/AST/TextNodeDumper.cpp
Index: cfe/trunk/lib/AST/TextNodeDumper.cpp
===
--- cfe/trunk/lib/AST/TextNodeDumper.cpp
+++ cfe/trunk/lib/AST/TextNodeDumper.cpp
@@ -272,6 +272,24 @@
}
}
+void TextNodeDumper::Visit(const OMPClause *C) {
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
+ {
+ColorScope Color(OS, ShowColors, AttrColor);
+StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
+OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
+ << ClauseName.drop_front() << "Clause";
+ }
+ dumpPointer(C);
+ dumpSourceRange(SourceRange(C->getBeginLoc(), C->getEndLoc()));
+ if (C->isImplicit())
+OS << " ";
+}
+
void TextNodeDumper::dumpPointer(const void *Ptr) {
ColorScope Color(OS, ShowColors, AddressColor);
OS << ' ' << Ptr;
Index: cfe/trunk/lib/AST/ASTDumper.cpp
===
--- cfe/trunk/lib/AST/ASTDumper.cpp
+++ cfe/trunk/lib/AST/ASTDumper.cpp
@@ -292,6 +292,7 @@
void VisitCapturedStmt(const CapturedStmt *Node);
// OpenMP
+void Visit(const OMPClause *C);
void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
// Exprs
@@ -1448,29 +1449,18 @@
// OpenMP dumping methods.
//===--===//
+void ASTDumper::Visit(const OMPClause *C) {
+ dumpChild([=] {
+NodeDumper.Visit(C);
+for (auto *S : C->children())
+ dumpStmt(S);
+ });
+}
+
void ASTDumper::VisitOMPExecutableDirective(
const OMPExecutableDirective *Node) {
- for (auto *C : Node->clauses()) {
-dumpChild([=] {
- if (!C) {
-ColorScope Color(OS, ShowColors, NullColor);
-OS << "<<>> OMPClause";
-return;
- }
- {
-ColorScope Color(OS, ShowColors, AttrColor);
-StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
-OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
- << ClauseName.drop_front() << "Clause";
- }
- NodeDumper.dumpPointer(C);
- NodeDumper.dumpSourceRange(SourceRange(C->getBeginLoc(),
C->getEndLoc()));
- if (C->isImplicit())
-OS << " ";
- for (auto *S : C->children())
-dumpStmt(S);
-});
- }
+ for (const auto *C : Node->clauses())
+Visit(C);
}
//===--===//
Index: cfe/trunk/include/clang/AST/TextNodeDumper.h
===
--- cfe/trunk/include/clang/AST/TextNodeDumper.h
+++ cfe/trunk/include/clang/AST/TextNodeDumper.h
@@ -169,6 +169,8 @@
void Visit(const CXXCtorInitializer *Init);
+ void Visit(const OMPClause *C);
+
void dumpPointer(const void *Ptr);
void dumpLocation(SourceLocation Loc);
void dumpSourceRange(SourceRange R);
Index: cfe/trunk/lib/AST/TextNodeDumper.cpp
===
--- cfe/trunk/lib/AST/TextNodeDumper.cpp
+++ cfe/trunk/lib/AST/TextNodeDumper.cpp
@@ -272,6 +272,24 @@
}
}
+void TextNodeDumper::Visit(const OMPClause *C) {
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
+ {
+ColorScope Color(OS, ShowColors, AttrColor);
+StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
+OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
+ << ClauseName.drop_front() << "Clause";
+ }
+ dumpPointer(C);
+ dumpSourceRange(SourceRange(C->getBeginLoc(), C->getEndLoc()));
+ if (C->isImplicit())
+OS << " ";
+}
+
void TextNodeDumper::dumpPointer(const void *Ptr) {
ColorScope Color(OS, ShowColors, AddressColor);
OS << ' ' << Ptr;
Index: cfe/trunk/lib/AST/ASTDumper.cpp
===
--- cfe/trunk/lib/AST/ASTDumper.cpp
+++ cfe/trunk/lib/AST/ASTDumper.cpp
@@ -292,6 +292,7 @@
void VisitCapturedStmt(const CapturedStmt *Node);
// OpenMP
+void Visit(const OMPClause *C);
void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
// Exprs
@@ -1448,29 +1449,18 @@
// OpenMP dumping methods.
//===--===//
+void ASTDumper::Visit(const OMPClause *C) {
+
[PATCH] D56708: NFC: Implement OMPClause dump in terms of visitors
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from a nit.
Comment at: lib/AST/ASTDumper.cpp:1462
const OMPExecutableDirective *Node) {
- for (auto *C : Node->clauses()) {
-dumpChild([=] {
- if (!C) {
-ColorScope Color(OS, ShowColors, NullColor);
-OS << "<<>> OMPClause";
-return;
- }
- {
-ColorScope Color(OS, ShowColors, AttrColor);
-StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
-OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
- << ClauseName.drop_front() << "Clause";
- }
- NodeDumper.dumpPointer(C);
- NodeDumper.dumpSourceRange(SourceRange(C->getBeginLoc(),
C->getEndLoc()));
- if (C->isImplicit())
-OS << " ";
- for (auto *S : C->children())
-dumpStmt(S);
-});
- }
+ for (auto *C : Node->clauses())
+Visit(C);
`const auto *`
Comment at: lib/AST/TextNodeDumper.cpp:276-280
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
This pattern is coming up quite frequently. I wonder if we should factor this
out into something like:
```
template
bool dumpNullObject(const Ty *Val, StringRef Label) const {
if (!Val) {
ColorScope Color(OS, ShowColors, NullColor);
OS << "<<>> " << Label;
}
return !Val;
}
```
So that uses become:
```
if (dumpNullObject(Whatever, "Whatever"))
return;
```
Doesn't have to be done as part of this patch, but it seems like it might
reduce some redundancy.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56708/new/
https://reviews.llvm.org/D56708
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56708: NFC: Implement OMPClause dump in terms of visitors
steveire updated this revision to Diff 181749.
steveire added a comment.
Update
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56708/new/
https://reviews.llvm.org/D56708
Files:
include/clang/AST/TextNodeDumper.h
lib/AST/ASTDumper.cpp
lib/AST/TextNodeDumper.cpp
Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -272,6 +272,24 @@
}
}
+void TextNodeDumper::Visit(const OMPClause *C) {
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
+ {
+ColorScope Color(OS, ShowColors, AttrColor);
+StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
+OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
+ << ClauseName.drop_front() << "Clause";
+ }
+ dumpPointer(C);
+ dumpSourceRange(SourceRange(C->getBeginLoc(), C->getEndLoc()));
+ if (C->isImplicit())
+OS << " ";
+}
+
void TextNodeDumper::dumpPointer(const void *Ptr) {
ColorScope Color(OS, ShowColors, AddressColor);
OS << ' ' << Ptr;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -292,6 +292,7 @@
void VisitCapturedStmt(const CapturedStmt *Node);
// OpenMP
+void Visit(const OMPClause *C);
void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
// Exprs
@@ -1448,29 +1449,18 @@
// OpenMP dumping methods.
//===--===//
+void ASTDumper::Visit(const OMPClause *C) {
+ dumpChild([=] {
+NodeDumper.Visit(C);
+for (auto *S : C->children())
+ dumpStmt(S);
+ });
+}
+
void ASTDumper::VisitOMPExecutableDirective(
const OMPExecutableDirective *Node) {
- for (auto *C : Node->clauses()) {
-dumpChild([=] {
- if (!C) {
-ColorScope Color(OS, ShowColors, NullColor);
-OS << "<<>> OMPClause";
-return;
- }
- {
-ColorScope Color(OS, ShowColors, AttrColor);
-StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
-OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
- << ClauseName.drop_front() << "Clause";
- }
- NodeDumper.dumpPointer(C);
- NodeDumper.dumpSourceRange(SourceRange(C->getBeginLoc(),
C->getEndLoc()));
- if (C->isImplicit())
-OS << " ";
- for (auto *S : C->children())
-dumpStmt(S);
-});
- }
+ for (auto *C : Node->clauses())
+Visit(C);
}
//===--===//
Index: include/clang/AST/TextNodeDumper.h
===
--- include/clang/AST/TextNodeDumper.h
+++ include/clang/AST/TextNodeDumper.h
@@ -169,6 +169,8 @@
void Visit(const CXXCtorInitializer *Init);
+ void Visit(const OMPClause *C);
+
void dumpPointer(const void *Ptr);
void dumpLocation(SourceLocation Loc);
void dumpSourceRange(SourceRange R);
Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -272,6 +272,24 @@
}
}
+void TextNodeDumper::Visit(const OMPClause *C) {
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
+ {
+ColorScope Color(OS, ShowColors, AttrColor);
+StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
+OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
+ << ClauseName.drop_front() << "Clause";
+ }
+ dumpPointer(C);
+ dumpSourceRange(SourceRange(C->getBeginLoc(), C->getEndLoc()));
+ if (C->isImplicit())
+OS << " ";
+}
+
void TextNodeDumper::dumpPointer(const void *Ptr) {
ColorScope Color(OS, ShowColors, AddressColor);
OS << ' ' << Ptr;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -292,6 +292,7 @@
void VisitCapturedStmt(const CapturedStmt *Node);
// OpenMP
+void Visit(const OMPClause *C);
void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
// Exprs
@@ -1448,29 +1449,18 @@
// OpenMP dumping methods.
//===--===//
+void ASTDumper::Visit(const OMPClause *C) {
+ dumpChild([=] {
+NodeDumper.Visit(C);
+for (auto *S : C->children())
+ dumpStmt(S);
+ });
+}
+
void ASTDumper::VisitOMPExecutableDirective(
const OMPExecutableDirective *Node) {
- for (auto *C : Node->clauses()) {
-dumpChild([=] {
- if (!C) {
-ColorScope Color(OS, ShowColors, NullColor);
-OS << "<<>> OMPClause";
-return;
- }
- {
-ColorScope Color(OS,
[PATCH] D56708: NFC: Implement OMPClause dump in terms of visitors
steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.
Repository:
rC Clang
https://reviews.llvm.org/D56708
Files:
include/clang/AST/TextNodeDumper.h
lib/AST/ASTDumper.cpp
lib/AST/TextNodeDumper.cpp
Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -272,6 +272,24 @@
}
}
+void TextNodeDumper::Visit(const OMPClause *C) {
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
+ {
+ColorScope Color(OS, ShowColors, AttrColor);
+StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
+OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
+ << ClauseName.drop_front() << "Clause";
+ }
+ dumpPointer(C);
+ dumpSourceRange(SourceRange(C->getBeginLoc(), C->getEndLoc()));
+ if (C->isImplicit())
+OS << " ";
+}
+
void TextNodeDumper::dumpPointer(const void *Ptr) {
ColorScope Color(OS, ShowColors, AddressColor);
OS << ' ' << Ptr;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -292,6 +292,7 @@
void VisitCapturedStmt(const CapturedStmt *Node);
// OpenMP
+void Visit(const OMPClause *C);
void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
// Exprs
@@ -1448,27 +1449,17 @@
// OpenMP dumping methods.
//===--===//
+void ASTDumper::Visit(const OMPClause *C) {
+ for (auto *S : C->children())
+dumpStmt(S);
+}
+
void ASTDumper::VisitOMPExecutableDirective(
const OMPExecutableDirective *Node) {
for (auto *C : Node->clauses()) {
dumpChild([=] {
- if (!C) {
-ColorScope Color(OS, ShowColors, NullColor);
-OS << "<<>> OMPClause";
-return;
- }
- {
-ColorScope Color(OS, ShowColors, AttrColor);
-StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
-OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
- << ClauseName.drop_front() << "Clause";
- }
- NodeDumper.dumpPointer(C);
- NodeDumper.dumpSourceRange(SourceRange(C->getBeginLoc(),
C->getEndLoc()));
- if (C->isImplicit())
-OS << " ";
- for (auto *S : C->children())
-dumpStmt(S);
+ NodeDumper.Visit(C);
+ Visit(C);
});
}
}
Index: include/clang/AST/TextNodeDumper.h
===
--- include/clang/AST/TextNodeDumper.h
+++ include/clang/AST/TextNodeDumper.h
@@ -169,6 +169,8 @@
void Visit(const CXXCtorInitializer *Init);
+ void Visit(const OMPClause *C);
+
void dumpPointer(const void *Ptr);
void dumpLocation(SourceLocation Loc);
void dumpSourceRange(SourceRange R);
Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -272,6 +272,24 @@
}
}
+void TextNodeDumper::Visit(const OMPClause *C) {
+ if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>> OMPClause";
+return;
+ }
+ {
+ColorScope Color(OS, ShowColors, AttrColor);
+StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
+OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
+ << ClauseName.drop_front() << "Clause";
+ }
+ dumpPointer(C);
+ dumpSourceRange(SourceRange(C->getBeginLoc(), C->getEndLoc()));
+ if (C->isImplicit())
+OS << " ";
+}
+
void TextNodeDumper::dumpPointer(const void *Ptr) {
ColorScope Color(OS, ShowColors, AddressColor);
OS << ' ' << Ptr;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -292,6 +292,7 @@
void VisitCapturedStmt(const CapturedStmt *Node);
// OpenMP
+void Visit(const OMPClause *C);
void VisitOMPExecutableDirective(const OMPExecutableDirective *Node);
// Exprs
@@ -1448,27 +1449,17 @@
// OpenMP dumping methods.
//===--===//
+void ASTDumper::Visit(const OMPClause *C) {
+ for (auto *S : C->children())
+dumpStmt(S);
+}
+
void ASTDumper::VisitOMPExecutableDirective(
const OMPExecutableDirective *Node) {
for (auto *C : Node->clauses()) {
dumpChild([=] {
- if (!C) {
-ColorScope Color(OS, ShowColors, NullColor);
-OS << "<<>> OMPClause";
-return;
- }
- {
-ColorScope Color(OS, ShowColors, AttrColor);
-StringRef ClauseName(getOpenMPClauseName(C->getClauseKind()));
-OS << "OMP" << ClauseName.substr(/*Start=*/0, /*N=*/1).upper()
- << ClauseName.drop_front() << "Clause";
- }
-
