aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/Sarif.h:229 +/// used to create an empty shell onto which attributes can be added using the +/// \c setX(...) methods. The +/// ---------------- No idea what clang-format wants done here, but there's a stray `The` at the end of the line. ================ Comment at: clang/include/clang/Basic/Sarif.h:95 +// +/// Since every in clang artifact MUST have a location (there being no nested +/// artifacts), the creation method \ref SarifArtifact::create requires a ---------------- vaibhav.y wrote: > aaron.ballman wrote: > > How do we expect to handle artifact locations that don't correspond > > directly to a file? For example, the user can specify macros on the command > > line and those macros could have a diagnostic result associated with them. > > Can we handle that sort of scenario? > I hadn't considered `-D` macros. Definitely a valid case to handle. I will > respond with more information once I read through the related portions of the > SARIF spec. A (sort of hacky?) solution come to mind after a quick glance: > > Setting the artifact URI to: `data:text/plain:<CLI_ARG>` with an offset, and > saying that it's role is `"referencedOnCommandLine"`. It seems strange since > we need to copy over the command line, instead of referencing it directly. > What do you think: > https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317617 > > Dropping a TODO comment so I can update later. > > `TODO(envp)` From my reading of SARIF, I think that sounds plausible. At least, I didn't see anything else that would work. ================ Comment at: clang/lib/Basic/Sarif.cpp:230-232 + if (!hasRun()) { + return; + } ---------------- vaibhav.y wrote: > aaron.ballman wrote: > > Is there a reason why we don't want to assert instead? > Creating a document requires ending any ongoing runs, and it is valid to > create a document without any runs, so `createDocument()` calls `endRun()`. > > I guess having a flag to mark the status of the document, and only calling > `endRun()` if a an active run exists would likely be better. (`hasRun()` > seems to have a rather broad responsibility, tracking both the availability & > state of the current run) > > I'm thinking of adding a `Closed` flag to the writer (default `true`), which > is unset whenever `createRun()` is called, and `endRun()` will set the same > flag. That way we only `endRun()` if there is something to end, like so: > > 1. Constructor makes writer with `Closed = true` > 2. `createRun()` requires `Closed == true`, and sets it to `false` > 3. `endRun()` requires `Closed == false`, and sets it to `true` > 4. `createDocument()` requires `Closed == true`, and will call `endRun()` to > ensure that > > What do you think? I think that makes sense. Adding assertions where appropriate would also help users who misuse the interface catch their issues earlier rather than later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
