[clangd] Explicitly initialize all primitive fields in Protocol.h

[ilya-biryukov]

Summary

Some of the existing structs had primimtive fields that were
not explicitly initialized on construction.
After this commit every struct consistently sets a defined value for
every field when default-initialized.

[ilya-biryukov]

This commit makes potential UB less likely. Logical errors (i.e. forgot to initialize some fields) will still be there.
Sending this for review to make sure everyone agrees this is a good thing.

[ilya-biryukov]

• Removed initializers for Optional<> fields, they are not problematic

[jkorous-apple]

clangd/Protocol.h

190 ↗

(On Diff #134018)

Sorry if I am asking stupid question but since the type is Optional<> isn't it's default-constructed value more appropriate here?

211 ↗

(On Diff #134018)

Does it still make more sense to use Optional<TraceLevel> than plain TraceLevel?

[ioeric]

I think it's useful to explicit initialize non-trivial types like FileChangeType. But I think it's safe and probably easier to rely on default values of primitive types like int, bool etc. Explicitly setting default values is probably fine (so this change LGTM), but do we really want to make this a requirement for future changes or even in our coding style?

[ilya-biryukov]

clangd/Protocol.h

190 ↗

(On Diff #134018)

Totally, my mistake. Fixed now.

211 ↗

(On Diff #134018)

Optional here makes sense (LSP defines the field as nullable), the intializer didn't. Fixed now.

[ilya-biryukov]

In D43230#1006104, @ioeric wrote:

But I think it's safe and probably easier to rely on default values of primitive types like int, bool etc

It's not always safe, as primitive types are sometimes left uninitialized (e.g. when constructed on the stack) and reading an uninitialized value is UB.

but do we really want to make this a requirement for future changes or even in our coding style?

I think we should, default values are less surprising than UB. Other people may disagree, though.
@sammccall , @hokein , WDYT? Should we always initialize primitive types in our code?

[ioeric]

In D43230#1006469, @ilya-biryukov wrote:

In D43230#1006104, @ioeric wrote:

But I think it's safe and probably easier to rely on default values of primitive types like int, bool etc

It's not always safe, as primitive types are sometimes left uninitialized (e.g. when constructed on the stack) and reading an uninitialized value is UB.

Oh, you are absolutely right! I I think l had protos get into my mind...

but do we really want to make this a requirement for future changes or even in our coding style?

I think we should, default values are less surprising than UB. Other people may disagree, though.
@sammccall , @hokein , WDYT? Should we always initialize primitive types in our code?

I think it would probably depend on whether we could find a sensible default value for a field (e.g. is 0 a better default value than UINT_MAX for line number?) and whether we expect users to always initialize a field (e.g. would it be better to pollute the field instead of providing a default value so that uninitialized values would be caught more easily).

[sammccall]

Yup, I got bitten recently from some of our plain-c-style structs with no default initializers (in Index).

Definitely a fan of this change. Main downside is you can't use aggregate initialization, but the field-by-field initialization is often more readable anyway.

And +1 to avoiding explicit = None for optionals, etc.

[sammccall]

clangd/Protocol.h

80 ↗

(On Diff #134019)

I'd lean to making callers initialize field-by-field here rather than provide constructors.
It's not terrible here (you can get it wrong, but only one way), but it's a pattern that doesn't scale so well I think.

[ilya-biryukov]

• Removed initializers for Optional<> fields, they are not problematic
• Remove ctor from Position, initialize on per-field basis instead.

[ilya-biryukov]

In D43230#1006498, @ioeric wrote:

I think it would probably depend on whether we could find a sensible default value for a field (e.g. is 0 a better default value than UINT_MAX for line number?) and whether we expect users to always initialize a field (e.g. would it be better to pollute the field instead of providing a default value so that uninitialized values would be caught more easily).

Yeah, it arguable whether 0 is the right default, but at least it's consistent with what compiler uses for zero-initialization, i.e. when you default-initialize builtin types explicitly (e.g. int b = int(); or int b = *(new int)).
For things like FileChangeType we choose 1, because it's the lowest acceptable value, but I'm not sure that's the right default value, merely a random valid enum value.

clangd/Protocol.h

80 ↗

(On Diff #134019)

As discussed offline, removed the ctor from Position and initialized everything on per-field basis.

It strikes me that this should be decided on per-struct basis. For things like position per-field init seems preferable to avoid accidentally confusing lines and chars.
On the other hand, for Range it seems totally fine to have a constructor, it's almost impossible to write Range{end, begin}, the ordering of ctor params is very natural.

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

if (!Edits.empty()) {
           {"arguments", {WE}},
       });
     }
   }
   reply(std::move(Commands));
 }
 
 void ClangdLSPServer::onCompletion(TextDocumentPositionParams &Params) {
-  Server.codeComplete(Params.textDocument.uri.file,
-                      Position{Params.position.line, Params.position.character},
-                      CCOpts,
+  Server.codeComplete(Params.textDocument.uri.file, Params.position, CCOpts,
                       [](Tagged<CompletionList> List) { reply(List.Value); });
 }
 
 void ClangdLSPServer::onSignatureHelp(TextDocumentPositionParams &Params) {
-  auto SignatureHelp = Server.signatureHelp(
-      Params.textDocument.uri.file,
-      Position{Params.position.line, Params.position.character});
+  auto SignatureHelp =
+      Server.signatureHelp(Params.textDocument.uri.file, Params.position);
   if (!SignatureHelp)
     return replyError(ErrorCode::InvalidParams,
                       llvm::toString(SignatureHelp.takeError()));
   reply(SignatureHelp->Value);
 }
 
 void ClangdLSPServer::onGoToDefinition(TextDocumentPositionParams &Params) {
-  auto Items = Server.findDefinitions(
-      Params.textDocument.uri.file,
-      Position{Params.position.line, Params.position.character});
+  auto Items =
+      Server.findDefinitions(Params.textDocument.uri.file, Params.position);
   if (!Items)
     return replyError(ErrorCode::InvalidParams,
                       llvm::toString(Items.takeError()));
   reply(json::ary(Items->Value));
 }
 
 void ClangdLSPServer::onSwitchSourceHeader(TextDocumentIdentifier &Params) {
   llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file);
   reply(Result ? URI::createFile(*Result).toString() : "");
 }
 
 void ClangdLSPServer::onDocumentHighlight(TextDocumentPositionParams &Params) {
-  auto Highlights = Server.findDocumentHighlights(
-      Params.textDocument.uri.file,
-      Position{Params.position.line, Params.position.character});
+  auto Highlights = Server.findDocumentHighlights(Params.textDocument.uri.file,
+                                                  Params.position);
 
   if (!Highlights) {
     replyError(ErrorCode::InternalError,
                llvm::toString(Highlights.takeError()));
     return;
   }
 
   reply(json::ary(Highlights->Value));

clang-tools-extra/trunk/clangd/ClangdUnit.cpp

if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
     return false;
   return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
 }
 
 // Converts a half-open clang source range to an LSP range.
 // Note that clang also uses closed source ranges, which this can't handle!
 Range toRange(CharSourceRange R, const SourceManager &M) {
   // Clang is 1-based, LSP uses 0-based indexes.
-  return {{static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1,
-           static_cast<int>(M.getSpellingColumnNumber(R.getBegin())) - 1},
-          {static_cast<int>(M.getSpellingLineNumber(R.getEnd())) - 1,
-           static_cast<int>(M.getSpellingColumnNumber(R.getEnd())) - 1}};
+  Position Begin;
+  Begin.line = static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1;
+  Begin.character =
+      static_cast<int>(M.getSpellingColumnNumber(R.getBegin())) - 1;
+
+  Position End;
+  End.line = static_cast<int>(M.getSpellingLineNumber(R.getEnd())) - 1;
+  End.character = static_cast<int>(M.getSpellingColumnNumber(R.getEnd())) - 1;
+
+  return {Begin, End};
 }
 
 // Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
 // LSP needs a single range.
 Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   auto &M = D.getSourceManager();
   auto Loc = M.getFileLoc(D.getLocation());
   // Accept the first range that contains the location.
SourceLocation clangd::getBeginningOfIdentifier(ParsedAST &Unit,
   // This handle cases where the position is in the middle of a token or right
   // after the end of a token. In theory we could just use GetBeginningOfToken
   // to find the start of the token at the input position, but this doesn't
   // work when right after the end, i.e. foo|.
   // So try to go back by one and see if we're still inside an identifier
   // token. If so, Take the beginning of this token.
   // (It should be the same identifier because you can't have two adjacent
   // identifiers without another token in between.)
-  SourceLocation PeekBeforeLocation = getMacroArgExpandedLocation(
-      SourceMgr, FE, Position{Pos.line, Pos.character - 1});
+  Position PosCharBehind = Pos;
+  --PosCharBehind.character;
+
+  SourceLocation PeekBeforeLocation =
+      getMacroArgExpandedLocation(SourceMgr, FE, PosCharBehind);
   Token Result;
   if (Lexer::getRawToken(PeekBeforeLocation, Result, SourceMgr,
                          AST.getLangOpts(), false)) {
     // getRawToken failed, just use InputLocation.
     return InputLocation;
   }
 
   if (Result.is(tok::raw_identifier)) {
     return Lexer::GetBeginningOfToken(PeekBeforeLocation, SourceMgr,
                                       AST.getLangOpts());
   }
 
   return InputLocation;
 }

clang-tools-extra/trunk/clangd/Protocol.h

 struct TextDocumentIdentifier {
   /// The text document's URI.
   URIForFile uri;
 };
 bool fromJSON(const json::Expr &, TextDocumentIdentifier &);
 
 struct Position {
   /// Line position in a document (zero-based).
-  int line;
+  int line = 0;
 
   /// Character offset on a line in a document (zero-based).
-  int character;
+  int character = 0;
 
   friend bool operator==(const Position &LHS, const Position &RHS) {
     return std::tie(LHS.line, LHS.character) ==
            std::tie(RHS.line, RHS.character);
   }
   friend bool operator<(const Position &LHS, const Position &RHS) {
     return std::tie(LHS.line, LHS.character) <
            std::tie(RHS.line, RHS.character);

 struct TextDocumentItem {
   /// The text document's URI.
   URIForFile uri;
 
   /// The text document's language identifier.
   std::string languageId;
 
   /// The version number of this document (it will strictly increase after each
-  int version;
+  int version = 0;
 
   /// The content of the opened text document.
   std::string text;
 };
 bool fromJSON(const json::Expr &, TextDocumentItem &);
 
 enum class TraceLevel {
   Off = 0,
enum class FileChangeType {
   Deleted = 3
 };
 bool fromJSON(const json::Expr &E, FileChangeType &Out);
 
 struct FileEvent {
   /// The file's URI.
   URIForFile uri;
   /// The change type.
-  FileChangeType type;
+  FileChangeType type = FileChangeType::Created;
 };
 bool fromJSON(const json::Expr &, FileEvent &);
 
 struct DidChangeWatchedFilesParams {
   /// The actual file events.
   std::vector<FileEvent> changes;
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
 struct FormattingOptions {
   /// Size of a tab in spaces.
-  int tabSize;
+  int tabSize = 0;
 
   /// Prefer spaces over tabs.
-  bool insertSpaces;
+  bool insertSpaces = false;
 };
 bool fromJSON(const json::Expr &, FormattingOptions &);
 json::Expr toJSON(const FormattingOptions &);
 
 struct DocumentRangeFormattingParams {
   /// The document to format.
   TextDocumentIdentifier textDocument;
 

 bool fromJSON(const json::Expr &, DocumentFormattingParams &);
 
 struct Diagnostic {
   /// The range at which the message applies.
   Range range;
 
   /// The diagnostic's severity. Can be omitted. If omitted it is up to the
   /// client to interpret diagnostics as error, warning, info or hint.
-  int severity;
+  int severity = 0;
 
   /// The diagnostic's code. Can be omitted.
   /// Note: Not currently used by clangd
   // std::string code;
 
   /// A human-readable string describing the source of this
   /// diagnostic, e.g. 'typescript' or 'super lint'.
   /// Note: Not currently used by clangd

 struct ExecuteCommandParams {
   // Command to apply fix-its. Uses WorkspaceEdit as argument.
   const static llvm::StringLiteral CLANGD_APPLY_FIX_COMMAND;
 
   /// The command identifier, e.g. CLANGD_APPLY_FIX_COMMAND
   std::string command;
 
   // Arguments
-
   llvm::Optional<WorkspaceEdit> workspaceEdit;
 };
 bool fromJSON(const json::Expr &, ExecuteCommandParams &);
 
 struct ApplyWorkspaceEditParams {
   WorkspaceEdit edit;
 };
 json::Expr toJSON(const ApplyWorkspaceEditParams &);
enum class InsertTextFormat {
   Snippet = 2,
 };
 
 /// Provides details for how a completion item was scored.
 /// This can be used for client-side filtering of completion items as the
 /// user keeps typing.
 /// This is a clangd extension.
 struct CompletionItemScores {
-  float finalScore;  /// The score that items are ranked by.
-                     /// This is filterScore * symbolScore.
-  float filterScore; /// How the partial identifier matched filterText. [0-1]
-  float symbolScore; /// How the symbol fits, ignoring the partial identifier.
+  /// The score that items are ranked by.
+  float finalScore = 0.f;
+  /// How the partial identifier matched filterText. [0-1]
+  float filterScore = 0.f;
+  /// How the symbol fits, ignoring the partial identifier.
+  float symbolScore = 0.f;
 };
 
 struct CompletionItem {
   /// The label of this completion item. By default also the text that is
   /// inserted when selecting this completion.
   std::string label;
 
   /// The kind of this completion item. Based of the kind an icon is chosen by

 
 enum class DocumentHighlightKind { Text = 1, Read = 2, Write = 3 };
 
 /// A document highlight is a range inside a text document which deserves
 /// special attention. Usually a document highlight is visualized by changing
 /// the background color of its range.
 
 struct DocumentHighlight {
-
   /// The range this highlight applies to.
-
   Range range;
 
   /// The highlight kind, default is DocumentHighlightKind.Text.
-
   DocumentHighlightKind kind = DocumentHighlightKind::Text;
 
   friend bool operator<(const DocumentHighlight &LHS,
                         const DocumentHighlight &RHS) {
     int LHSKind = static_cast<int>(LHS.kind);
     int RHSKind = static_cast<int>(RHS.kind);
     return std::tie(LHS.range, LHSKind) < std::tie(RHS.range, RHSKind);
   }

clang-tools-extra/trunk/clangd/SourceCode.cpp

 
 Position offsetToPosition(StringRef Code, size_t Offset) {
   Offset = std::min(Code.size(), Offset);
   StringRef Before = Code.substr(0, Offset);
   int Lines = Before.count('\n');
   size_t PrevNL = Before.rfind('\n');
   size_t StartOfLine = (PrevNL == StringRef::npos) ? 0 : (PrevNL + 1);
   // FIXME: officially character counts UTF-16 code units, not UTF-8 bytes!
-  return {Lines, static_cast<int>(Offset - StartOfLine)};
+  Position Pos;
+  Pos.line = Lines;
+  Pos.character = static_cast<int>(Offset - StartOfLine);
+  return Pos;
 }
 
 } // namespace clangd
 } // namespace clang
 

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

TEST_F(ClangdVFSTest, CheckVersions) {
 
   // Use default completion options.
   clangd::CodeCompleteOptions CCOpts;
 
   // No need to sync reparses, because requests are processed on the calling
   // thread.
   FS.Tag = "123";
   Server.addDocument(FooCpp, SourceContents);
-  EXPECT_EQ(runCodeComplete(Server, FooCpp, Position{0, 0}, CCOpts).Tag,
-            FS.Tag);
+  EXPECT_EQ(runCodeComplete(Server, FooCpp, Position(), CCOpts).Tag, FS.Tag);
   EXPECT_EQ(DiagConsumer.lastVFSTag(), FS.Tag);
 
   FS.Tag = "321";
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_EQ(DiagConsumer.lastVFSTag(), FS.Tag);
-  EXPECT_EQ(runCodeComplete(Server, FooCpp, Position{0, 0}, CCOpts).Tag,
-            FS.Tag);
+  EXPECT_EQ(runCodeComplete(Server, FooCpp, Position(), CCOpts).Tag, FS.Tag);
 }
 
 // Only enable this test on Unix
 #ifdef LLVM_ON_UNIX
 TEST_F(ClangdVFSTest, SearchLibDir) {
   // Checks that searches for GCC installation is done through vfs.
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
TEST_F(ClangdVFSTest, InvalidCompileCommand) {
   // CompileCommand. We pass the file in ExtraFlags once and CDB adds another
   // one in getCompileCommand().
   CDB.ExtraClangFlags.push_back(FooCpp.str());
 
   // Clang can't parse command args in that case, but we shouldn't crash.
   Server.addDocument(FooCpp, "int main() {}");
 
   EXPECT_EQ(Server.dumpAST(FooCpp), "<no-ast>");
-  EXPECT_ERROR(Server.findDefinitions(FooCpp, Position{0, 0}));
-  EXPECT_ERROR(Server.findDocumentHighlights(FooCpp, Position{0, 0}));
-  EXPECT_ERROR(Server.rename(FooCpp, Position{0, 0}, "new_name"));
+  EXPECT_ERROR(Server.findDefinitions(FooCpp, Position()));
+  EXPECT_ERROR(Server.findDocumentHighlights(FooCpp, Position()));
+  EXPECT_ERROR(Server.rename(FooCpp, Position(), "new_name"));
   // FIXME: codeComplete and signatureHelp should also return errors when they
   // can't parse the file.
-  EXPECT_THAT(runCodeComplete(Server, FooCpp, Position{0, 0},
-                              clangd::CodeCompleteOptions())
-                  .Value.items,
-              IsEmpty());
-  auto SigHelp = Server.signatureHelp(FooCpp, Position{0, 0});
+  EXPECT_THAT(
+      runCodeComplete(Server, FooCpp, Position(), clangd::CodeCompleteOptions())
+  auto SigHelp = Server.signatureHelp(FooCpp, Position());
   ASSERT_TRUE(bool(SigHelp)) << "signatureHelp returned an error";
   EXPECT_THAT(SigHelp->Value.signatures, IsEmpty());
 }
 
 class ClangdThreadingTest : public ClangdVFSTest {};
 
 TEST_F(ClangdThreadingTest, StressTest) {
   // Without 'static' clang gives an error for a usage inside TestDiagConsumer.
TestDiagConsumer DiagConsumer;
     };
 
     auto CodeCompletionRequest = [&]() {
       unsigned FileIndex = FileIndexDist(RandGen);
       // Make sure we don't violate the ClangdServer's contract.
       if (ReqStats[FileIndex].FileIsRemoved)
         AddDocument(FileIndex);
 
-      Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
+      Position Pos;
+      Pos.line = LineDist(RandGen);
+      Pos.character = ColumnDist(RandGen);
       // FIXME(ibiryukov): Also test async completion requests.
       // Simply putting CodeCompletion into async requests now would make
       // tests slow, since there's no way to cancel previous completion
       // requests as opposed to AddDocument/RemoveDocument, which are implicitly
       // cancelled by any subsequent AddDocument/RemoveDocument request to the
       // same file.
       runCodeComplete(Server, FilePaths[FileIndex], Pos,
                       clangd::CodeCompleteOptions());
     };
 
     auto FindDefinitionsRequest = [&]() {
       unsigned FileIndex = FileIndexDist(RandGen);
       // Make sure we don't violate the ClangdServer's contract.
       if (ReqStats[FileIndex].FileIsRemoved)
         AddDocument(FileIndex);
 
-      Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
+      Position Pos;
+      Pos.line = LineDist(RandGen);
+      Pos.character = ColumnDist(RandGen);
+
       ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
     };
 
     std::vector<std::function<void()>> AsyncRequests = {
         AddDocumentRequest, ForceReparseRequest, RemoveDocumentRequest};
     std::vector<std::function<void()>> BlockingRequests = {
         CodeCompletionRequest, FindDefinitionsRequest};
 

clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp

 MATCHER_P2(Pos, Line, Col, "") {
   return arg.line == Line && arg.character == Col;
 }
 
 const char File[] = R"(0:0 = 0
 1:0 = 8
 2:0 = 16)";
 
+/// A helper to make tests easier to read.
+Position position(int line, int character) {
+  Position Pos;
+  Pos.line = line;
+  Pos.character = character;
+  return Pos;
+}
+
 TEST(SourceCodeTests, PositionToOffset) {
   // line out of bounds
-  EXPECT_EQ(0u, positionToOffset(File, Position{-1, 2}));
+  EXPECT_EQ(0u, positionToOffset(File, position(-1, 2)));
   // first line
-  EXPECT_EQ(0u, positionToOffset(File, Position{0, -1})); // out of range
-  EXPECT_EQ(0u, positionToOffset(File, Position{0, 0})); // first character
-  EXPECT_EQ(3u, positionToOffset(File, Position{0, 3})); // middle character
-  EXPECT_EQ(6u, positionToOffset(File, Position{0, 6})); // last character
-  EXPECT_EQ(7u, positionToOffset(File, Position{0, 7})); // the newline itself
-  EXPECT_EQ(8u, positionToOffset(File, Position{0, 8})); // out of range
+  EXPECT_EQ(0u, positionToOffset(File, position(0, -1))); // out of range
+  EXPECT_EQ(0u, positionToOffset(File, position(0, 0)));  // first character
+  EXPECT_EQ(3u, positionToOffset(File, position(0, 3)));  // middle character
+  EXPECT_EQ(6u, positionToOffset(File, position(0, 6)));  // last character
+  EXPECT_EQ(7u, positionToOffset(File, position(0, 7)));  // the newline itself
+  EXPECT_EQ(8u, positionToOffset(File, position(0, 8)));  // out of range
   // middle line
-  EXPECT_EQ(8u, positionToOffset(File, Position{1, -1})); // out of range
-  EXPECT_EQ(8u, positionToOffset(File, Position{1, 0})); // first character
-  EXPECT_EQ(11u, positionToOffset(File, Position{1, 3})); // middle character
-  EXPECT_EQ(14u, positionToOffset(File, Position{1, 6})); // last character
-  EXPECT_EQ(15u, positionToOffset(File, Position{1, 7})); // the newline itself
-  EXPECT_EQ(16u, positionToOffset(File, Position{1, 8})); // out of range
+  EXPECT_EQ(8u, positionToOffset(File, position(1, -1))); // out of range
+  EXPECT_EQ(8u, positionToOffset(File, position(1, 0)));  // first character
+  EXPECT_EQ(11u, positionToOffset(File, position(1, 3))); // middle character
+  EXPECT_EQ(14u, positionToOffset(File, position(1, 6))); // last character
+  EXPECT_EQ(15u, positionToOffset(File, position(1, 7))); // the newline itself
+  EXPECT_EQ(16u, positionToOffset(File, position(1, 8))); // out of range
   // last line
-  EXPECT_EQ(16u, positionToOffset(File, Position{2, -1})); // out of range
-  EXPECT_EQ(16u, positionToOffset(File, Position{2, 0})); // first character
-  EXPECT_EQ(19u, positionToOffset(File, Position{2, 3})); // middle character
-  EXPECT_EQ(23u, positionToOffset(File, Position{2, 7})); // last character
-  EXPECT_EQ(24u, positionToOffset(File, Position{2, 8})); // EOF
-  EXPECT_EQ(24u, positionToOffset(File, Position{2, 9})); // out of range
+  EXPECT_EQ(16u, positionToOffset(File, position(2, -1))); // out of range
+  EXPECT_EQ(16u, positionToOffset(File, position(2, 0)));  // first character
+  EXPECT_EQ(19u, positionToOffset(File, position(2, 3)));  // middle character
+  EXPECT_EQ(23u, positionToOffset(File, position(2, 7)));  // last character
+  EXPECT_EQ(24u, positionToOffset(File, position(2, 8)));  // EOF
+  EXPECT_EQ(24u, positionToOffset(File, position(2, 9)));  // out of range
   // line out of bounds
-  EXPECT_EQ(24u, positionToOffset(File, Position{3, 1}));
+  EXPECT_EQ(24u, positionToOffset(File, position(3, 1)));
 }
 
 TEST(SourceCodeTests, OffsetToPosition) {
   EXPECT_THAT(offsetToPosition(File, 0), Pos(0, 0)) << "start of file";
   EXPECT_THAT(offsetToPosition(File, 3), Pos(0, 3)) << "in first line";
   EXPECT_THAT(offsetToPosition(File, 6), Pos(0, 6)) << "end of first line";
   EXPECT_THAT(offsetToPosition(File, 7), Pos(0, 7)) << "first newline";
   EXPECT_THAT(offsetToPosition(File, 8), Pos(1, 0)) << "start of second line";