Skip to content

Commit 12fe847

Browse files
committed
Responding to Brian's excellent pull request comments.
1 parent 05eeb9e commit 12fe847

File tree

1 file changed

+40
-37
lines changed

1 file changed

+40
-37
lines changed

documentation/ImplementingASemanticCheck.md

Lines changed: 40 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<!--===- documentation/FrontEndTutorial.md
1+
<!--===- documentation/ImplementingASemanticCheck.md
22
33
Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
44
See https://llvm.org/LICENSE.txt for license information.
@@ -61,7 +61,7 @@ of the call to `intentOutFunc()`:
6161

6262
I also used this program to produce a parse tree for the program using the command:
6363
```bash
64-
./bin/f18 -fdebug-dump-parse-tree -fparse-only testfun.f90
64+
f18 -fdebug-dump-parse-tree -fparse-only testfun.f90
6565
```
6666

6767
Here's the relevant fragment of the parse tree produced by the compiler:
@@ -111,9 +111,9 @@ checking had already taken place.
111111
Most semantic checks for statements are implemented by walking the parse tree
112112
and performing analysis on the nodes they visit. My plan was to use this
113113
method. The infrastructure for walking the parse tree for statement semantic
114-
checking is implemented in the files .../lib/semantics/semantics.[cc,h].
114+
checking is implemented in the files `lib/semantics/semantics.cpp`.
115115
Here's a fragment of the declaration of the framework's parse tree visitor from
116-
.../lib/semantics/semantics.cc:
116+
`lib/semantics/semantics.cpp`:
117117

118118
```C++
119119
// A parse tree visitor that calls Enter/Leave functions from each checker
@@ -136,7 +136,7 @@ Here's a fragment of the declaration of the framework's parse tree visitor from
136136
Since FUNCTION calls are a kind of expression, I was planning to base my
137137
implementation on the contents of `parser::Expr` nodes. I would need to define
138138
either an `Enter()` or `Leave()` function whose parameter was a `parser::Expr`
139-
node. Here's the declaration I put into check-do.h:
139+
node. Here's the declaration I put into `lib/semantics/check-do.h`:
140140
141141
```C++
142142
void Leave(const parser::Expr &);
@@ -148,12 +148,12 @@ arbitrarily chose to implement the `Leave()` function to visit the parse tree
148148
node.
149149

150150
Since my semantic check was focused on DO CONCURRENT statements, I added it to
151-
the file .../lib/semantics/check-do.cc where most of the semantic checking for
151+
the file `lib/semantics/check-do.cpp` where most of the semantic checking for
152152
DO statements already lived.
153153

154154
## Taking advantage of prior work
155155
When implementing a similar check for SUBROUTINE calls, I created a utility
156-
functions in .../lib/semantics/semantics.[cc.h] to emit messages if
156+
functions in `lib/semantics/semantics.cpp` to emit messages if
157157
a symbol corresponding to an active DO variable was being potentially modified:
158158

159159
```C++
@@ -175,21 +175,22 @@ functions. The second is needed to determine whether to call them.
175175
176176
## Finding the source location
177177
The source code location information that I'd need for the error message must
178-
come from the parse tree. I looked in the file .../lib/parser/parse-tree.h and
179-
determined that a `struct Expr` contained source location information since
180-
it had the field `CharBlock source`. Thus, if I visited a
181-
`parser::Expr` node, I could get the source location information for the
182-
associated expression.
178+
come from the parse tree. I looked in the file
179+
`include/flang/parser/parse-tree.h` and determined that a `struct Expr`
180+
contained source location information since it had the field `CharBlock
181+
source`. Thus, if I visited a `parser::Expr` node, I could get the source
182+
location information for the associated expression.
183183
184184
## Determining the `INTENT`
185185
I knew that I could find the `INTENT` of the dummy argument associated with the
186186
actual argument from the function called `dummyIntent()` in the class
187-
`evaluate::ActualArgument` in the file .../lib/evaluate/call.h. So if I could
188-
find an `evaluate::ActualArgument` in an expression, I could determine the
189-
`INTENT` of the associated dummy argument. I knew that it was valid to call
190-
`dummyIntent()` because the data on which `dummyIntent()` depends is
191-
established during semantic processing for expressions, and the semantic
192-
processing for expressions happens before semantic checking for DO constructs.
187+
`evaluate::ActualArgument` in the file `include/flang/evaluate/call.h`. So
188+
if I could find an `evaluate::ActualArgument` in an expression, I could
189+
determine the `INTENT` of the associated dummy argument. I knew that it was
190+
valid to call `dummyIntent()` because the data on which `dummyIntent()`
191+
depends is established during semantic processing for expressions, and the
192+
semantic processing for expressions happens before semantic checking for DO
193+
constructs.
193194
194195
In my prior work on checking the INTENT of arguments for SUBROUTINE calls,
195196
the parse tree held a node for the call (a `parser::CallStmt`) that contained
@@ -210,11 +211,11 @@ For a FUNCTION call, though, there is no similar way to get from a parse tree
210211
node to an `evaluate::ProcedureRef` node. But I knew that there was an
211212
existing framework used in DO construct semantic checking that traversed an
212213
`evaluate::Expr` node collecting `semantics::Symbol` nodes. I guessed that I'd
213-
be able to use a similar framework to traverse an ` evaluate::Expr` node to
214+
be able to use a similar framework to traverse an `evaluate::Expr` node to
214215
find all of the `evaluate::ActualArgument` nodes.
215216

216-
All of the declarations associated with both FUNCTION and SUBROUTINE
217-
calls are in .../lib/evaluate/call.h. An `evaluate::FunctionRef` inherits from
217+
All of the declarations associated with both FUNCTION and SUBROUTINE calls are
218+
in `include/flang/evaluate/call.h`. An `evaluate::FunctionRef` inherits from
218219
an `evaluate::ProcedureRef` which contains the list of
219220
`evaluate::ActualArgument` nodes. But the relationship between an
220221
`evaluate::FunctionRef` node and its associated arguments is not relevant. I
@@ -252,16 +253,16 @@ argument was an active DO variable.
252253
## Adding a parse tree visitor
253254
I started my implementation by adding a visitor for `parser::Expr` nodes.
254255
Since this analysis is part of DO construct checking, I did this in
255-
.../lib/semantics/check-do.[cc,h]. I added a print statement to the visitor to
256+
`lib/semantics/check-do.cpp`. I added a print statement to the visitor to
256257
verify that my new code was actually getting executed.
257258
258-
In check-do.h, I added the declaration for the visitor:
259+
In `lib/semantics/check-do.h`, I added the declaration for the visitor:
259260
260261
```C++
261262
void Leave(const parser::Expr &);
262263
```
263264

264-
In check-do.cc, I added an (almost empty) implementation:
265+
In `lib/semantics/check-do.cpp`, I added an (almost empty) implementation:
265266

266267
```C++
267268
void DoChecker::Leave(const parser::Expr &) {
@@ -272,7 +273,7 @@ In check-do.cc, I added an (almost empty) implementation:
272273
I then built the compiler with these changes and ran it on my test program.
273274
This time, I made sure to invoke semantic checking. Here's the command I used:
274275
```bash
275-
./bin/f18 -fdebug-resolve-names -fdebug-dump-parse-tree -funparse-with-symbols testfun.f90
276+
f18 -fdebug-resolve-names -fdebug-dump-parse-tree -funparse-with-symbols testfun.f90
276277
```
277278

278279
This produced the output:
@@ -299,7 +300,7 @@ framework to walk the `evaluate::Expr` to gather all of the
299300
`evaluate::ActualArgument` nodes. The code that I planned to model it on
300301
was the existing infrastructure that collected all of the `semantics::Symbol` nodes from an
301302
`evaluate::Expr`. I found this implementation in
302-
.../lib/evaluate/tools.cc:
303+
`lib/evaluate/tools.cpp`:
303304

304305
```C++
305306
struct CollectSymbolsHelper
@@ -316,7 +317,8 @@ was the existing infrastructure that collected all of the `semantics::Symbol` no
316317
}
317318
```
318319
319-
Note that the `CollectSymbols()` function returns a `semantics::Symbolset`, which is declared in .../lib/semantics/symbol.h:
320+
Note that the `CollectSymbols()` function returns a `semantics::Symbolset`,
321+
which is declared in `include/flang/semantics/symbol.h`:
320322
321323
```C++
322324
using SymbolSet = std::set<SymbolRef>;
@@ -338,10 +340,11 @@ full `semantics::Symbol` objects into the set. Ideally, we would be able to cre
338340
`std::set<Symbol &>` (a set of C++ references to symbols). But C++ doesn't
339341
support sets that contain references. This limitation is part of the rationale
340342
for the f18 implementation of type `common::Reference`, which is defined in
341-
.../lib/common/reference.h.
343+
`include/flang/common/reference.h`.
342344
343345
`SymbolRef`, the specialization of the template `common::Reference` for
344-
`semantics::Symbol`, is declared in the file .../lib/semantics/symbol.h:
346+
`semantics::Symbol`, is declared in the file
347+
`include/flang/semantics/symbol.h`:
345348
346349
```C++
347350
using SymbolRef = common::Reference<const Symbol>;
@@ -351,7 +354,7 @@ So to implement something that would collect `evaluate::ActualArgument`
351354
nodes from an `evaluate::Expr`, I first defined the required types
352355
`ActualArgumentRef` and `ActualArgumentSet`. Since these are being
353356
used exclusively for DO construct semantic checking (currently), I put their
354-
definitions into .../lib/semantics/check-do.cc:
357+
definitions into `lib/semantics/check-do.cpp`:
355358

356359

357360
```C++
@@ -367,7 +370,7 @@ Since `ActualArgument` is in the namespace `evaluate`, I put the
367370
definition for `ActualArgumentRef` in that namespace, too.
368371
369372
I then modeled the code to create an `ActualArgumentSet` after the code to
370-
collect a `SymbolSet` and put it into check-do.cc:
373+
collect a `SymbolSet` and put it into `lib/semantics/check-do.cpp`:
371374
372375
373376
```C++
@@ -506,8 +509,8 @@ symbol table node (`semantics::Symbol`) for the variable. My starting point was
506509
`evaluate::ActualArgument` node.
507510

508511
I was unsure of how to do this, so I browsed through existing code to look for
509-
how it treated `evaluate::ActualArgument` objects. Since most of the code that deals with the `evaluate` namespace is in the .../lib/evaluate directory, I looked there. I ran `grep` on all of the `.cc` files looking for
510-
uses of `ActualArgument`. One of the first hits I got was in .../lib/evaluate/call.cc in the definition of `ActualArgument::GetType()`:
512+
how it treated `evaluate::ActualArgument` objects. Since most of the code that deals with the `evaluate` namespace is in the lib/evaluate directory, I looked there. I ran `grep` on all of the `.cpp` files looking for
513+
uses of `ActualArgument`. One of the first hits I got was in `lib/evaluate/call.cpp` in the definition of `ActualArgument::GetType()`:
511514

512515
```C++
513516
std::optional<DynamicType> ActualArgument::GetType() const {
@@ -525,9 +528,9 @@ I noted the call to `UnwrapExpr()` that yielded a value of
525528
`Expr<SomeType>`. So I guessed that I could use this member function to
526529
get an `evaluate::Expr<SomeType>` on which I could perform further analysis.
527530

528-
I also knew that the header file .../lib/evaluate/tools.h held many utility
529-
functions for dealing with `evaluate::Expr` objects. I was hoping to find
530-
something that would determine if an `evaluate::Expr` was a variable. So
531+
I also knew that the header file `include/flang/evaluate/tools.h` held many
532+
utility functions for dealing with `evaluate::Expr` objects. I was hoping to
533+
find something that would determine if an `evaluate::Expr` was a variable. So
531534
I searched for `IsVariable` and got a hit immediately.
532535
```C++
533536
template<typename A> bool IsVariable(const A &x) {
@@ -541,7 +544,7 @@ I searched for `IsVariable` and got a hit immediately.
541544
542545
But I actually needed more than just the knowledge that an `evaluate::Expr` was
543546
a variable. I needed the `semantics::Symbol` associated with the variable. So
544-
I searched in .../lib/evaluate/tools.h for functions that returned a
547+
I searched in `include/flang/evaluate/tools.h` for functions that returned a
545548
`semantics::Symbol`. I found the following:
546549
547550
```C++

0 commit comments

Comments
 (0)