Skip to content

Commit

Permalink
[clang] Make deprecations of some FileManager APIs formal (llvm#110014
Browse files Browse the repository at this point in the history
)

Some `FileManager` APIs still return `{File,Directory}Entry` instead of
the preferred `{File,Directory}EntryRef`. These are documented to be
deprecated, but don't have the attribute that warns on their usage. This
PR marks them as such with `LLVM_DEPRECATED()` and replaces their usage
with the recommended counterparts. NFCI.
  • Loading branch information
jansvoboda11 authored Sep 25, 2024
1 parent abe0dd1 commit b1aea98
Show file tree
Hide file tree
Showing 28 changed files with 135 additions and 129 deletions.
2 changes: 1 addition & 1 deletion clang-tools-extra/clang-move/tool/ClangMove.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ int main(int argc, const char **argv) {
for (auto I = Files.begin(), E = Files.end(); I != E; ++I) {
OS << " {\n";
OS << " \"FilePath\": \"" << *I << "\",\n";
const auto Entry = FileMgr.getFile(*I);
const auto Entry = FileMgr.getOptionalFileRef(*I);
auto ID = SM.translateFile(*Entry);
std::string Content;
llvm::raw_string_ostream ContentStream(Content);
Expand Down
4 changes: 2 additions & 2 deletions clang-tools-extra/clangd/SourceCode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -814,8 +814,8 @@ llvm::SmallVector<llvm::StringRef> ancestorNamespaces(llvm::StringRef NS) {

// Checks whether \p FileName is a valid spelling of main file.
bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) {
auto FE = SM.getFileManager().getFile(FileName);
return FE && *FE == SM.getFileEntryForID(SM.getMainFileID());
auto FE = SM.getFileManager().getOptionalFileRef(FileName);
return FE && FE == SM.getFileEntryRefForID(SM.getMainFileID());
}

} // namespace
Expand Down
4 changes: 2 additions & 2 deletions clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,10 @@ TEST(ParsedASTTest, PatchesAdditionalIncludes) {
auto &FM = SM.getFileManager();
// Copy so that we can use operator[] to get the children.
IncludeStructure Includes = PatchedAST->getIncludeStructure();
auto MainFE = FM.getFile(testPath("foo.cpp"));
auto MainFE = FM.getOptionalFileRef(testPath("foo.cpp"));
ASSERT_TRUE(MainFE);
auto MainID = Includes.getID(*MainFE);
auto AuxFE = FM.getFile(testPath("sub/aux.h"));
auto AuxFE = FM.getOptionalFileRef(testPath("sub/aux.h"));
ASSERT_TRUE(AuxFE);
auto AuxID = Includes.getID(*AuxFE);
EXPECT_THAT(Includes.IncludeChildren[*MainID], Contains(*AuxID));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class FindHeadersTest : public testing::Test {
llvm::SmallVector<Hinted<Header>> findHeaders(llvm::StringRef FileName) {
return include_cleaner::findHeaders(
AST->sourceManager().translateFileLineCol(
AST->fileManager().getFile(FileName).get(),
*AST->fileManager().getOptionalFileRef(FileName),
/*Line=*/1, /*Col=*/1),
AST->sourceManager(), &PI);
}
Expand Down
84 changes: 42 additions & 42 deletions clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ TEST_F(RecordPPTest, CapturesMacroRefs) {
const auto &SM = AST.sourceManager();

SourceLocation Def = SM.getComposedLoc(
SM.translateFile(AST.fileManager().getFile("header.h").get()),
SM.translateFile(*AST.fileManager().getOptionalFileRef("header.h")),
Header.point("def"));
ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));
Symbol OrigX = Recorded.MacroReferences.front().Target;
Expand Down Expand Up @@ -368,29 +368,29 @@ TEST_F(PragmaIncludeTest, IWYUKeep) {
TestAST Processed = build();
auto &FM = Processed.fileManager();

EXPECT_FALSE(PI.shouldKeep(FM.getFile("normal.h").get()));
EXPECT_FALSE(PI.shouldKeep(FM.getFile("std/vector").get()));
EXPECT_FALSE(PI.shouldKeep(*FM.getOptionalFileRef("normal.h")));
EXPECT_FALSE(PI.shouldKeep(*FM.getOptionalFileRef("std/vector")));

// Keep
EXPECT_TRUE(PI.shouldKeep(FM.getFile("keep1.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("keep2.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("keep3.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("keep4.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("keep5.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("keep6.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("std/map").get()));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("keep1.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("keep2.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("keep3.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("keep4.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("keep5.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("keep6.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("std/map")));

// Exports
EXPECT_TRUE(PI.shouldKeep(FM.getFile("export1.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("export2.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("export3.h").get()));
EXPECT_TRUE(PI.shouldKeep(FM.getFile("std/set").get()));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("export1.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("export2.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("export3.h")));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("std/set")));
}

TEST_F(PragmaIncludeTest, AssociatedHeader) {
createEmptyFiles({"foo/main.h", "bar/main.h", "bar/other.h", "std/vector"});
auto IsKeep = [&](llvm::StringRef Name, TestAST &AST) {
return PI.shouldKeep(AST.fileManager().getFile(Name).get());
return PI.shouldKeep(*AST.fileManager().getOptionalFileRef(Name));
};

Inputs.FileName = "main.cc";
Expand Down Expand Up @@ -452,19 +452,19 @@ TEST_F(PragmaIncludeTest, IWYUPrivate) {
// IWYU pragma: private
)cpp";
TestAST Processed = build();
auto PrivateFE = Processed.fileManager().getFile("private.h");
auto PrivateFE = Processed.fileManager().getOptionalFileRef("private.h");
assert(PrivateFE);
EXPECT_TRUE(PI.isPrivate(PrivateFE.get()));
EXPECT_EQ(PI.getPublic(PrivateFE.get()), "\"public2.h\"");
EXPECT_TRUE(PI.isPrivate(*PrivateFE));
EXPECT_EQ(PI.getPublic(*PrivateFE), "\"public2.h\"");

auto PublicFE = Processed.fileManager().getFile("public.h");
auto PublicFE = Processed.fileManager().getOptionalFileRef("public.h");
assert(PublicFE);
EXPECT_EQ(PI.getPublic(PublicFE.get()), ""); // no mapping.
EXPECT_FALSE(PI.isPrivate(PublicFE.get()));
EXPECT_EQ(PI.getPublic(*PublicFE), ""); // no mapping.
EXPECT_FALSE(PI.isPrivate(*PublicFE));

auto Private2FE = Processed.fileManager().getFile("private2.h");
auto Private2FE = Processed.fileManager().getOptionalFileRef("private2.h");
assert(Private2FE);
EXPECT_TRUE(PI.isPrivate(Private2FE.get()));
EXPECT_TRUE(PI.isPrivate(*Private2FE));
}

TEST_F(PragmaIncludeTest, IWYUExport) {
Expand All @@ -486,13 +486,13 @@ TEST_F(PragmaIncludeTest, IWYUExport) {
const auto &SM = Processed.sourceManager();
auto &FM = Processed.fileManager();

EXPECT_THAT(PI.getExporters(FM.getFile("private.h").get(), FM),
EXPECT_THAT(PI.getExporters(*FM.getOptionalFileRef("private.h"), FM),
testing::UnorderedElementsAre(FileNamed("export1.h"),
FileNamed("export3.h")));

EXPECT_TRUE(PI.getExporters(FM.getFile("export1.h").get(), FM).empty());
EXPECT_TRUE(PI.getExporters(FM.getFile("export2.h").get(), FM).empty());
EXPECT_TRUE(PI.getExporters(FM.getFile("export3.h").get(), FM).empty());
EXPECT_TRUE(PI.getExporters(*FM.getOptionalFileRef("export1.h"), FM).empty());
EXPECT_TRUE(PI.getExporters(*FM.getOptionalFileRef("export2.h"), FM).empty());
EXPECT_TRUE(PI.getExporters(*FM.getOptionalFileRef("export3.h"), FM).empty());
EXPECT_TRUE(
PI.getExporters(SM.getFileEntryForID(SM.getMainFileID()), FM).empty());
}
Expand Down Expand Up @@ -548,23 +548,23 @@ TEST_F(PragmaIncludeTest, IWYUExportBlock) {
}
return Result;
};
auto Exporters = PI.getExporters(FM.getFile("private1.h").get(), FM);
auto Exporters = PI.getExporters(*FM.getOptionalFileRef("private1.h"), FM);
EXPECT_THAT(Exporters, testing::UnorderedElementsAre(FileNamed("export1.h"),
FileNamed("normal.h")))
<< GetNames(Exporters);

Exporters = PI.getExporters(FM.getFile("private2.h").get(), FM);
Exporters = PI.getExporters(*FM.getOptionalFileRef("private2.h"), FM);
EXPECT_THAT(Exporters, testing::UnorderedElementsAre(FileNamed("export1.h")))
<< GetNames(Exporters);

Exporters = PI.getExporters(FM.getFile("private3.h").get(), FM);
Exporters = PI.getExporters(*FM.getOptionalFileRef("private3.h"), FM);
EXPECT_THAT(Exporters, testing::UnorderedElementsAre(FileNamed("export1.h")))
<< GetNames(Exporters);

Exporters = PI.getExporters(FM.getFile("foo.h").get(), FM);
Exporters = PI.getExporters(*FM.getOptionalFileRef("foo.h"), FM);
EXPECT_TRUE(Exporters.empty()) << GetNames(Exporters);

Exporters = PI.getExporters(FM.getFile("bar.h").get(), FM);
Exporters = PI.getExporters(*FM.getOptionalFileRef("bar.h"), FM);
EXPECT_TRUE(Exporters.empty()) << GetNames(Exporters);
}

Expand All @@ -580,8 +580,8 @@ TEST_F(PragmaIncludeTest, SelfContained) {
Inputs.ExtraFiles["unguarded.h"] = "";
TestAST Processed = build();
auto &FM = Processed.fileManager();
EXPECT_TRUE(PI.isSelfContained(FM.getFile("guarded.h").get()));
EXPECT_FALSE(PI.isSelfContained(FM.getFile("unguarded.h").get()));
EXPECT_TRUE(PI.isSelfContained(*FM.getOptionalFileRef("guarded.h")));
EXPECT_FALSE(PI.isSelfContained(*FM.getOptionalFileRef("unguarded.h")));
}

TEST_F(PragmaIncludeTest, AlwaysKeep) {
Expand All @@ -596,8 +596,8 @@ TEST_F(PragmaIncludeTest, AlwaysKeep) {
Inputs.ExtraFiles["usual.h"] = "#pragma once";
TestAST Processed = build();
auto &FM = Processed.fileManager();
EXPECT_TRUE(PI.shouldKeep(FM.getFile("always_keep.h").get()));
EXPECT_FALSE(PI.shouldKeep(FM.getFile("usual.h").get()));
EXPECT_TRUE(PI.shouldKeep(*FM.getOptionalFileRef("always_keep.h")));
EXPECT_FALSE(PI.shouldKeep(*FM.getOptionalFileRef("usual.h")));
}

TEST_F(PragmaIncludeTest, ExportInUnnamedBuffer) {
Expand Down Expand Up @@ -653,13 +653,13 @@ TEST_F(PragmaIncludeTest, OutlivesFMAndSM) {
// Now this build gives us a new File&Source Manager.
TestAST Processed = build(/*ResetPragmaIncludes=*/false);
auto &FM = Processed.fileManager();
auto PrivateFE = FM.getFile("private.h");
auto PrivateFE = FM.getOptionalFileRef("private.h");
assert(PrivateFE);
EXPECT_EQ(PI.getPublic(PrivateFE.get()), "\"public.h\"");
EXPECT_EQ(PI.getPublic(*PrivateFE), "\"public.h\"");

auto Private2FE = FM.getFile("private2.h");
auto Private2FE = FM.getOptionalFileRef("private2.h");
assert(Private2FE);
EXPECT_THAT(PI.getExporters(Private2FE.get(), FM),
EXPECT_THAT(PI.getExporters(*Private2FE, FM),
testing::ElementsAre(llvm::cantFail(FM.getFileRef("public.h"))));
}

Expand All @@ -676,8 +676,8 @@ TEST_F(PragmaIncludeTest, CanRecordManyTimes) {

TestAST Processed = build();
auto &FM = Processed.fileManager();
auto PrivateFE = FM.getFile("private.h");
llvm::StringRef Public = PI.getPublic(PrivateFE.get());
auto PrivateFE = FM.getOptionalFileRef("private.h");
llvm::StringRef Public = PI.getPublic(*PrivateFE);
EXPECT_EQ(Public, "\"public.h\"");

// This build populates same PI during build, but this time we don't have
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class VirtualFileHelper {
I != E; ++I) {
std::unique_ptr<llvm::MemoryBuffer> Buf =
llvm::MemoryBuffer::getMemBuffer(I->Code);
const FileEntry *Entry = SM.getFileManager().getVirtualFile(
FileEntryRef Entry = SM.getFileManager().getVirtualFileRef(
I->FileName, Buf->getBufferSize(), /*ModificationTime=*/0);
SM.overrideFileContents(Entry, std::move(Buf));
}
Expand Down
2 changes: 0 additions & 2 deletions clang/include/clang/Basic/DiagnosticFrontendKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ def err_fe_expected_clang_command : Error<
"expected a clang compiler command">;
def err_fe_remap_missing_to_file : Error<
"could not remap file '%0' to the contents of file '%1'">, DefaultFatal;
def err_fe_remap_missing_from_file : Error<
"could not remap from missing file '%0'">, DefaultFatal;
def err_fe_unable_to_load_pch : Error<
"unable to load PCH file">;
def err_fe_unable_to_load_plugin : Error<
Expand Down
8 changes: 7 additions & 1 deletion clang/include/clang/Basic/FileManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class FileManager : public RefCountedBase<FileManager> {
/// VirtualDirectoryEntries/VirtualFileEntries above.
///
llvm::StringMap<llvm::ErrorOr<DirectoryEntry &>, llvm::BumpPtrAllocator>
SeenDirEntries;
SeenDirEntries;

/// A cache that maps paths to file entries (either real or
/// virtual) we have looked up, or an error that occurred when we looked up
Expand Down Expand Up @@ -190,6 +190,8 @@ class FileManager : public RefCountedBase<FileManager> {
///
/// \param CacheFailure If true and the file does not exist, we'll cache
/// the failure to find this file.
LLVM_DEPRECATED("Functions returning DirectoryEntry are deprecated.",
"getOptionalDirectoryRef()")
llvm::ErrorOr<const DirectoryEntry *>
getDirectory(StringRef DirName, bool CacheFailure = true);

Expand All @@ -207,6 +209,8 @@ class FileManager : public RefCountedBase<FileManager> {
///
/// \param CacheFailure If true and the file does not exist, we'll cache
/// the failure to find this file.
LLVM_DEPRECATED("Functions returning FileEntry are deprecated.",
"getOptionalFileRef()")
llvm::ErrorOr<const FileEntry *>
getFile(StringRef Filename, bool OpenFile = false, bool CacheFailure = true);

Expand Down Expand Up @@ -269,6 +273,8 @@ class FileManager : public RefCountedBase<FileManager> {
FileEntryRef getVirtualFileRef(StringRef Filename, off_t Size,
time_t ModificationTime);

LLVM_DEPRECATED("Functions returning FileEntry are deprecated.",
"getVirtualFileRef()")
const FileEntry *getVirtualFile(StringRef Filename, off_t Size,
time_t ModificationTime);

Expand Down
4 changes: 2 additions & 2 deletions clang/lib/AST/ASTImporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10020,8 +10020,8 @@ Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
ToIncludeLocOrFakeLoc = ToSM.getLocForStartOfFile(ToSM.getMainFileID());

if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
// FIXME: We probably want to use getVirtualFile(), so we don't hit the
// disk again
// FIXME: We probably want to use getVirtualFileRef(), so we don't hit
// the disk again
// FIXME: We definitely want to re-use the existing MemoryBuffer, rather
// than mmap the files several times.
auto Entry =
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/CodeGen/CodeGenAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -586,9 +586,9 @@ const FullSourceLoc BackendConsumer::getBestLocationFromDebugLoc(
if (D.isLocationAvailable()) {
D.getLocation(Filename, Line, Column);
if (Line > 0) {
auto FE = FileMgr.getFile(Filename);
auto FE = FileMgr.getOptionalFileRef(Filename);
if (!FE)
FE = FileMgr.getFile(D.getAbsolutePath());
FE = FileMgr.getOptionalFileRef(D.getAbsolutePath());
if (FE) {
// If -gcolumn-info was not used, Column will be 0. This upsets the
// source manager, so pass 1 if Column is not set.
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,8 @@ struct LocationFileChecker {
SmallVector<std::pair<SmallString<32>, bool>> &KnownFiles)
: CI(CI), KnownFiles(KnownFiles), ExternalFileEntries() {
for (const auto &KnownFile : KnownFiles)
if (auto FileEntry = CI.getFileManager().getFile(KnownFile.first))
KnownFileEntries.insert(*FileEntry);
if (auto FE = CI.getFileManager().getOptionalFileRef(KnownFile.first))
KnownFileEntries.insert(*FE);
}

private:
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Frontend/ASTUnit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2395,7 +2395,7 @@ void ASTUnit::TranslateStoredDiagnostics(
// Rebuild the StoredDiagnostic.
if (SD.Filename.empty())
continue;
auto FE = FileMgr.getFile(SD.Filename);
auto FE = FileMgr.getOptionalFileRef(SD.Filename);
if (!FE)
continue;
SourceLocation FileLoc;
Expand Down
10 changes: 3 additions & 7 deletions clang/lib/Frontend/CompilerInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -427,12 +427,8 @@ static void InitializeFileRemapping(DiagnosticsEngine &Diags,
}

// Create the file entry for the file that we're mapping from.
const FileEntry *FromFile =
FileMgr.getVirtualFile(RF.first, ToFile->getSize(), 0);
if (!FromFile) {
Diags.Report(diag::err_fe_remap_missing_from_file) << RF.first;
continue;
}
FileEntryRef FromFile =
FileMgr.getVirtualFileRef(RF.first, ToFile->getSize(), 0);

// Override the contents of the "from" file with the contents of
// the "to" file.
Expand Down Expand Up @@ -1926,7 +1922,7 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(

// Check whether M refers to the file in the prebuilt module path.
if (M && M->getASTFile())
if (auto ModuleFile = FileMgr->getFile(ModuleFilename))
if (auto ModuleFile = FileMgr->getOptionalFileRef(ModuleFilename))
if (*ModuleFile == M->getASTFile())
return M;

Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Frontend/Rewrite/FrontendActions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ class RewriteIncludesAction::RewriteImportsListener : public ASTReaderListener {

void visitModuleFile(StringRef Filename,
serialization::ModuleKind Kind) override {
auto File = CI.getFileManager().getFile(Filename);
auto File = CI.getFileManager().getOptionalFileRef(Filename);
assert(File && "missing file for loaded module?");

// Only rewrite each module file once.
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/InstallAPI/Frontend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ InstallAPIContext::findAndRecordFile(const FileEntry *FE,
}

void InstallAPIContext::addKnownHeader(const HeaderFile &H) {
auto FE = FM->getFile(H.getPath());
auto FE = FM->getOptionalFileRef(H.getPath());
if (!FE)
return; // File does not exist.
KnownFiles[*FE] = H.getType();
Expand Down
8 changes: 4 additions & 4 deletions clang/lib/Lex/HeaderSearch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ std::string HeaderSearch::getPrebuiltModuleFileName(StringRef ModuleName,
".pcm");
else
llvm::sys::path::append(Result, ModuleName + ".pcm");
if (getFileMgr().getFile(Result.str()))
if (getFileMgr().getOptionalFileRef(Result))
return std::string(Result);
}

Expand All @@ -246,7 +246,7 @@ std::string HeaderSearch::getPrebuiltImplicitModuleFileName(Module *Module) {
llvm::sys::path::append(CachePath, ModuleCacheHash);
std::string FileName =
getCachedModuleFileNameImpl(ModuleName, ModuleMapPath, CachePath);
if (!FileName.empty() && getFileMgr().getFile(FileName))
if (!FileName.empty() && getFileMgr().getOptionalFileRef(FileName))
return FileName;
}
return {};
Expand Down Expand Up @@ -655,7 +655,7 @@ OptionalFileEntryRef DirectoryLookup::DoFrameworkLookup(
++NumFrameworkLookups;

// If the framework dir doesn't exist, we fail.
auto Dir = FileMgr.getDirectory(FrameworkName);
auto Dir = FileMgr.getOptionalDirectoryRef(FrameworkName);
if (!Dir)
return std::nullopt;

Expand Down Expand Up @@ -718,7 +718,7 @@ OptionalFileEntryRef DirectoryLookup::DoFrameworkLookup(
bool FoundFramework = false;
do {
// Determine whether this directory exists.
auto Dir = FileMgr.getDirectory(FrameworkPath);
auto Dir = FileMgr.getOptionalDirectoryRef(FrameworkPath);
if (!Dir)
break;

Expand Down
Loading

0 comments on commit b1aea98

Please sign in to comment.