aaron.ballman added a comment.
Thank you for working on this check, I think it's useful functionality. One
concern I have, though, is that it's not flow-sensitive and should probably be
implemented as a clang static analyzer check instead of a clang-tidy check. For
instance, consider these three plausible issues:
// This only sets the offset on one code path.
void func(FILE *fp) {
fpos_t offset;
if (condition) {
// ... code
if (0 != fgetpos(fp, &offset))
return;
// ... code
} else {
// ... code
}
fsetpos(fp, &offset);
}
// This doesn't check the failure from getting the position and sets the
position regardless.
void func(FILE *fp) {
fpos_t offset;
fgetpos(fp, &offset);
// ... code
fsetpos(fp, &offset);
}
// This function accepts the offset from the caller but the caller passes an
invalid offset.
void func(FILE *fp, const fpos_t *offset) {
fsetpos(fp, offset);
}
void caller(FILE *fp) {
fpos_t offset;
func(fp, &offset);
}
Have you considered writing a static analyzer check so you can do data and
control flow analysis to catch issues like these?
================
Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:103
// FIO
+
CheckFactories.registerCheck<bugprone::FsetposArgumentCheck>("cert-fio44-c");
CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>("cert-fio38-c");
----------------
Can you reorder to below fio38-c?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst:4
+bugprone-fsetpos-argument-check
+========================
+
----------------
The underlines here don't look like they are correct.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst:35
+
+This check corresponds to the CERT C++ Coding Standard rule
+`FIO44-C. Only use values for fsetpos() that are returned from fgetpos()
----------------
the CERT C Coding Standard rule
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79437/new/
https://reviews.llvm.org/D79437
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
