Skip to content

Commit

Permalink
internal: consider column for TLineInfo equality (#880)
Browse files Browse the repository at this point in the history
## Summary

Change the default equality operation for `TLineInfo` to also consider
the column, effectively changing the meaning of `TLineInfo` to "source
position info". Places in the compiler that relied on the old behaviour
are adjusted.

In most cases, a `TLineInfo` is already treated as a "source position",
so having the `==` operator reflect that makes sense. The `hash`
procedure also already considered the column, too.

## Details

The custom `==` implementation for `TLineInfo` is removed, which means
that the default implementation (value equality) is used. `exactEquals`
is redundant now: its usages are replaced with `==` and the procedure
itself is removed.

### Changed usage sites

The VM profiler implementation (`vmprofiler`) relied on the old equality
behaviour, as it uses `TLineInfo` as the key for a table that associates
profiler data with a source *line*. Here, `TLineInfo` usage is replaced
with the new `SourceLinePosition` tuple, which is a `TLineInfo` without
the column information.

For rendering the profiler data, a temporary `TLineInfo` instance is
created, but with the column set to -1. This means that the text output
now links to the source line *without* including a column position.

### Other relevant usage sites

There are two other places where the different equality behaviour
causes a visible change in compiler behaviour, but that are not
adjusted because the new behaviour is better:
- with reporting "nil dereference" warnings (`nilcheck.derefWarning`):
  warnings are now reported one per line+column, instead of one per
  line
- providing error/warning context (`msgs.getContext`, "instantiated
  from" messages): if there are multiple surrounding instantiation
  contexts on the same line (but in different columns), they are now all
  shown, instead of only the first one

---------

Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
  • Loading branch information
bung87 and zerbina authored Sep 10, 2023
1 parent 978fb2b commit 4b370a9
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 22 deletions.
6 changes: 6 additions & 0 deletions compiler/ast/lineinfos.nim
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ type
col*: int16
fileIndex*: FileIndex

SourceLinePosition* = tuple
## Identifies a line in a source file. Only intended for use by
## the profiler.
fileIndex: typeof(TLineInfo.fileIndex)
line: typeof(TLineInfo.line)

LineColPair* = tuple
line: typeof(TLineInfo.line)
col: typeof(TLineInfo.col)
Expand Down
7 changes: 0 additions & 7 deletions compiler/front/msgs.nim
Original file line number Diff line number Diff line change
Expand Up @@ -315,13 +315,6 @@ proc errorActions(
elif eh == doRaise:
result = (doRaise, false)


proc `==`*(a, b: TLineInfo): bool =
result = a.line == b.line and a.fileIndex == b.fileIndex

proc exactEquals*(a, b: TLineInfo): bool =
result = a.fileIndex == b.fileIndex and a.line == b.line and a.col == b.col

proc getContext*(conf: ConfigRef; lastinfo: TLineInfo): seq[ReportContext] =
## Get list of context context entries from the current message context
## information. Context messages can later be used in the
Expand Down
4 changes: 2 additions & 2 deletions compiler/front/options.nim
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ type
count*: int

ProfileData* = ref object
data*: TableRef[TLineInfo, ProfileInfo]
data*: TableRef[SourceLinePosition, ProfileInfo]

StdOrrKind* = enum
stdOrrStdout
Expand Down Expand Up @@ -774,7 +774,7 @@ template newPackageCache*(): untyped =
modeCaseSensitive)

proc newProfileData(): ProfileData =
ProfileData(data: newTable[TLineInfo, ProfileInfo]())
ProfileData(data: newTable[SourceLinePosition, ProfileInfo]())

proc isDefined*(conf: ConfigRef; symbol: string): bool

Expand Down
2 changes: 1 addition & 1 deletion compiler/sem/semexprs.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1775,7 +1775,7 @@ proc builtinFieldAccess(c: PContext, n: PNode, flags: TExprFlags): PNode =
when defined(nimsuggest):
if c.config.cmd == cmdIdeTools:
suggestExpr(c, n)
if exactEquals(c.config.m.trackPos, n[1].info): suggestExprNoCheck(c, n)
if c.config.m.trackPos == n[1].info: suggestExprNoCheck(c, n)

let s = qualifiedLookUp(c, n, {checkAmbiguity, checkUndeclared, checkModule})
if s.isError:
Expand Down
2 changes: 1 addition & 1 deletion compiler/sem/semstmts.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1691,7 +1691,7 @@ proc semCase(c: PContext, n: PNode; flags: TExprFlags): PNode =
setCaseContextIdx(c, i)
var x = n[i]
when defined(nimsuggest):
if c.config.ideCmd == ideSug and exactEquals(c.config.m.trackPos, x.info) and caseTyp.kind == tyEnum:
if c.config.ideCmd == ideSug and c.config.m.trackPos == x.info and caseTyp.kind == tyEnum:
suggestEnum(c, x, caseTyp)
case x.kind
of nkOfBranch:
Expand Down
11 changes: 3 additions & 8 deletions compiler/tools/suggest.nim
Original file line number Diff line number Diff line change
Expand Up @@ -469,17 +469,12 @@ proc isTracked*(current, trackPos: TLineInfo, tokenLen: int): bool =
return true

when defined(nimsuggest):
# Since TLineInfo defined a == operator that doesn't include the column,
# we map TLineInfo to a unique int here for this lookup table:
proc infoToInt(info: TLineInfo): int64 =
info.fileIndex.int64 + info.line.int64 shl 32 + info.col.int64 shl 48

proc addNoDup(s: PSym; info: TLineInfo) =
# ensure nothing gets too slow:
if s.allUsages.len > 500: return
let infoAsInt = info.infoToInt
for infoB in s.allUsages:
if infoB.infoToInt == infoAsInt: return
if infoB == info: return
s.allUsages.add(info)

proc findUsages(g: ModuleGraph; info: TLineInfo; s: PSym; usageSym: var PSym) =
Expand All @@ -496,7 +491,7 @@ when defined(nimsuggest):
proc listUsages*(g: ModuleGraph; s: PSym) =
#echo "usages ", s.allUsages.len
for info in s.allUsages:
let x = if info == s.info and info.col == s.info.col: ideDef else: ideUse
let x = if info == s.info: ideDef else: ideUse
suggestResult(g.config, symToSuggest(g, s, isLocal=false, x, info, 100, PrefixMatch.None, false, 0))

proc findDefinition(g: ModuleGraph; info: TLineInfo; s: PSym; usageSym: var PSym) =
Expand Down Expand Up @@ -608,7 +603,7 @@ proc suggestExprNoCheck*(c: PContext, n: PNode) =
suggestQuit()

proc suggestExpr*(c: PContext, n: PNode) =
if exactEquals(c.config.m.trackPos, n.info): suggestExprNoCheck(c, n)
if c.config.m.trackPos == n.info: suggestExprNoCheck(c, n)

proc suggestDecl*(c: PContext, n: PNode; s: PSym) =
let attached = c.config.m.trackPosAttached
Expand Down
9 changes: 6 additions & 3 deletions compiler/vm/vmprofiler.nim
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ proc leaveImpl(prof: var Profiler, c: TCtx) {.noinline.} =
while frameIdx >= 0:
let frame = c.sframes[frameIdx]
if frame.prc != nil:
let li = frame.prc.info
let li = (frame.prc.info.fileIndex, frame.prc.info.line)
if li notin data:
data[li] = ProfileInfo()
data[li].time += tLeave - prof.tEnter
Expand All @@ -48,7 +48,7 @@ proc dump*(conf: ConfigRef, pd: ProfileData): string =
result = "\nprof: µs #instr location\n"
for i in 0..<32:
var infoMax: ProfileInfo
var flMax: TLineInfo
var flMax: SourceLinePosition
for fl, info in data:
if info.time > infoMax.time:
infoMax = info
Expand All @@ -57,5 +57,8 @@ proc dump*(conf: ConfigRef, pd: ProfileData): string =
break
result.add " " & align($int(infoMax.time * 1e6), 10) &
align($int(infoMax.count), 10) & " " &
conf.toFileLineCol(flMax) & "\n"
toMsgFilename(conf, newLineInfo(flMax.fileIndex,
flMax.line.int,
-1)) &
"\n"
data.del flMax

0 comments on commit 4b370a9

Please sign in to comment.