fix: address cpplint warnings across LogMonitor sources#225
Merged
Conversation
* boost json parser Signed-off-by: Charity Kathure <ckathure@microsoft.com> Co-authored-by: Charity Kathure <ckathure@microsoft.com>
Signed-off-by: Charity Kathure <ckathure@microsoft.com> * review: fix memory safety, error handling, and build improvements - Use unique_ptr for channels vector in handleEventLog to eliminate manual delete and prevent memory leak on early return paths - SanitizeJson no longer replaces log content with placeholder on failure; leaves str unchanged and logs via logWriter instead - getJsonStringCaseInsensitive adds required param (default false) so optional fields don't emit spurious errors; required fields (name, level, directory, providerName, providerGuid) now pass true - Fix inconsistent nlohmann::json vs json alias in handleETWLog/handleProcessLog - build.cmd: detect cmake from VS or vcpkg when not on PATH; use goto to avoid delayed expansion issues with nested if blocks - docs: remove leading space from "Information" level in README example --------- Signed-off-by: Charity Kathure <ckathure@microsoft.com> Co-authored-by: Charity Kathure <ckathure@microsoft.com> Co-authored-by: Bob Sira <Bob.Sira@microsoft.com>
The SDL pipeline was failing because nlohmann/json.hpp was not available when MSBuild compiled the project. Added vcpkg setup and nlohmann-json installation steps before the build step.
- Fix boolean attributes stored as wstring* instead of bool* in JsonProcessor, causing incorrect EventFormatMultiLine/StartAtOldestRecord values after parsing - Fix eventFormatMultiLine default from false to true (documented default) - Make channel level optional in EventLog source (defaults to Error) - Fix processSources to skip invalid sources and continue (resilient mode) - Add missing waitInSeconds parsing in handleFileLog - Fix path injection in Main.cpp: validate config filename and anchor to exe directory using GetModuleFileNameW/PathCombineW - Fix %s -> %hs format specifier for narrow string in EtwMonitor.cpp - Rename Utility string conversion functions to PascalCase - Fix CMake toolchain file setup to run before project() call - Fix output directory to use generator expression for multi-config layout - Add JsonProcessorTests covering case-insensitive keys, optional field defaults, waitInSeconds parsing, and invalid source resilience
The VS .sln build compiles sources by #including them into LogMonitorTests.cpp. JsonProcessor.cpp was missing from that list, causing an unresolved external symbol for ReadConfigFile(PWCHAR,...). - Add #include of JsonProcessor.cpp to LogMonitorTests.cpp - Add #include "JsonProcessor.h" in JsonProcessor.cpp so all function declarations (with correct defaults) are visible when the file is #included as a translation unit, fixing C3861/C2660 errors
Both x64 and x86 jobs now install nlohmann-json via vcpkg before building. --no-binarycaching and VCPKG_BINARY_SOURCES=clear prevent vcpkg from downloading PowerShell Core (causing 504 errors in CI).
- Enable C++17 in CMakeLists.txt (fixes STL4038 for <variant>) - Remove unused hr, bytesWritten, hSubscription, eventsCount, szBuf local variables (C4189) - Replace E_FAIL (HRESULT) returns from DWORD functions with ERROR_UNHANDLED_EXCEPTION (C4245) - Mark unused Buffer and Event parameters as commented-out (C4100) - Rename shadowed 'status' variable in EventMonitor to 'retryStatus' (C4456) - Replace wchar_t-to-char narrowing in FileMonitorUtilities with Utility::WStringToString (C4244) - Remove dead functions BufferCopy, BufferCopyAndSanitize, ClearBuffer from ProcessMonitor.cpp (C4505) - Replace deprecated std::wstring_convert with Win32 APIs (WideCharToMultiByte / MultiByteToWideChar) - Add missing #include directives and fix cpplint style issues in JsonProcessor.cpp/.h and Utility.cpp/.h - Minor test cleanup and build script fixes (bootstrap call, vcpkg flags)
…nings Move JsonProcessor.h before standard library headers in JsonProcessor.cpp and add Utility.h before standard headers in Utility.cpp to satisfy cpplint's required include ordering: own header, c system, c++ system, other.
…ibility - Case-insensitive lookup for LogConfig, logFormat, and sources keys to match previous parser behavior (configs using logconfig/LOGCONFIG etc. now work) - Case-insensitive source type matching (eventlog, FILE, etw all accepted) - Normalize directory path via ParseDirectoryValue before storing (/ to \, strip trailing slashes) and validate via IsValidSourceFile - Validate ETW providers array presence before processing; fix incorrect error message referencing 'channels' to reference 'providers' - Accept providerName OR providerGuid for ETW providers (either is sufficient); skip and warn on entries where both are empty - Downgrade missing logFormat from TraceError to TraceWarning (it is optional) - Fix cleanupAttributes to delete void* through correct types to avoid UB - Add findJsonKeyCaseInsensitive helper for object key lookup
- Fix header guards in pch.h, version.h, resource.h to use full-path style - Add NOLINT suppressions for build/include_subdir and build/include_order where precompiled header ordering cannot be changed - Add explicit includes (<map>, <vector>, <queue>, <string>, <memory>, <cstdio>) to headers that use those types directly - Indent public:/private: access specifiers +1 space inside class bodies - Remove blank lines after public: labels - Mark single-parameter JsonFileParser constructor explicit - Fix line-length and trailing-whitespace violations - Normalize inconsistent method indentation in FileMonitorUtilities.h
The file lives inside FileMonitor/, so including "FileMonitor/FileMonitorUtilities.h" causes MSVC to look for FileMonitor/FileMonitor/FileMonitorUtilities.h which does not exist. Use the bare filename instead since the compiler already searches the source file's own directory.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves cpplint violations flagged by the CI reviewdog check that were
not included in the v2.2.0-rc release commit.
pch.h,version.h, andresource.hto use thefull-path style required by cpplint (
LOGMONITOR_SRC_LOGMONITOR_*_H_)NOLINTsuppressions forbuild/include_subdirandbuild/include_orderwhere the precompiled-header ordering cannot bechanged without breaking implicit include dependencies
#includedirectives (<map>,<vector>,<queue>,<string>,<memory>,<cstdio>) to headers that use those types directlypublic:/private:access specifiers +1 space inside class bodiespublic:labelsJsonFileParser(const std::wstring&)constructorexplicitFileMonitorUtilities.hTest plan