docs: add CONTRIBUTING.md with review priorities and LLM guidance by oharboe · Pull Request #9662 · The-OpenROAD-Project/OpenROAD

oharboe · 2026-03-06T06:22:29Z

CONTRIBUTING.md is the standard location that LLMs and github and humans will pick up on.

Consolidate project review priorities derived from existing developer docs and maintainer review patterns into a standard CONTRIBUTING.md. Includes a section for AI/LLM code reviewers (e.g., Gemini Code Assist) to align automated reviews with project standards.

These guidelines for LLM and humans were extracted based upon review history in git.

CONTRIBUTING.md is the standard location that LLMs and github will pick up on. Consolidate project review priorities derived from existing developer docs and maintainer review patterns into a standard CONTRIBUTING.md. Includes a section for AI/LLM code reviewers (e.g., Gemini Code Assist) to align automated reviews with project standards. These guidelines for LLM and humans were extracted based upon review history in git. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>

gemini-code-assist

Code Review

This pull request introduces a CONTRIBUTING.md file, which consolidates contribution guidelines, review priorities, and instructions for both human and AI reviewers. This is a valuable addition for improving the contribution process. My review found a minor issue with an incorrect C++ syntax example in the C++ Style section, for which I've provided a correction.

_{Note: Security Review has been skipped due to the limited scope of the PR.}

gemini-code-assist · 2026-03-06T06:24:43Z

CONTRIBUTING.md

+- Use `""` for project headers, `<>` only for system headers.
+- Use range-based for loops, not iterator loops.
+- Use forward declarations instead of includes where possible.
+- Don't use `using namespace std`. Prefer `using namespace::symbol`.


The syntax using namespace::symbol is not valid C++. The likely intent is to recommend using declarations for specific symbols.

This seems to be copied from docs/contrib/CodingPractices.md which has the same issue. You may want to correct it there as well.

I suggest clarifying this point for better readability and correctness.

Suggested change

- Don't use `using namespace std`. Prefer `using namespace::symbol`.

- Don't use `using namespace std;`. Prefer `using` declarations for specific symbols (e.g., `using std::vector;`).

github-actions · 2026-03-06T06:25:48Z

clang-tidy review says "All clean, LGTM! 👍"

maliberty · 2026-03-06T07:06:55Z

Claude doesn't read it and when asked says which LLMs do:

The short answer: none of the major ones do automatically. The industry has converged on dedicated agent instruction files instead of repurposing CONTRIBUTING.md.

oharboe · 2026-03-06T07:09:06Z

Claude doesn't read it and when asked says which LLMs do:

The short answer: none of the major ones do automatically. The industry has converged on dedicated agent instruction files instead of repurposing CONTRIBUTING.md.

I don't think I have access to those instructions. Can you update the instructions to read CONTRIBUTING.md as part of the review process?

maliberty · 2026-03-06T07:20:04Z

I'm not clear what you don't have access to. You state "for AI/LLM code review tools assisting with pull
request reviews" which I don't think is true as the LLMs won't read this file.

oharboe · 2026-03-06T08:40:57Z

@vvbandeira @maliberty unrelated CI network outage.

oharboe · 2026-03-06T10:40:36Z

I'm not clear what you don't have access to. You state "for AI/LLM code review tools assisting with pull request reviews" which I don't think is true as the LLMs won't read this file.

I should be able to point the gemini instructions to read this file when reviewing, no?

oharboe · 2026-03-06T10:40:59Z

LLMs aside, are these instructions helpful or just spam?

maliberty · 2026-03-06T14:11:51Z

You could manually tell your LLM to read the file but it isn't somehow automatic LLM guidance (cf CLAUDE.md)

oharboe · 2026-03-06T14:14:36Z

You could manually tell your LLM to read the file but it isn't somehow automatic LLM guidance (cf CLAUDE.md)

Quite, so locally I can say "review my code to be consistent with best practices" and Bob's your uncle, but no CI Gemini support without directing Gemini to it.

Are these practices helpful/accurate or just noise?

maliberty · 2026-03-06T14:13:02Z

CONTRIBUTING.md

+physical design metrics must be validated.
+
+- Ask: "Does this have a QoR impact?" Changes affecting QoR require
+  secure-CI runs on real designs, not just unit tests.


Most users won't know what a secure-CI run is (nor can they run it). Perhaps this needs its own section.

maliberty · 2026-03-06T14:13:36Z

CONTRIBUTING.md

+- Regression tests must be runnable from any directory and must
+  not leave the source tree dirty.
+- Regression output should be concise -- not thousands of lines.
+- Tests should pass `-no_init` to `openroad`.


Usually this is handled by the test runner

maliberty · 2026-03-06T14:14:33Z

CONTRIBUTING.md

+  `TritonCTS`).
+- No global variables. All state in classes.
+- No magic numbers. Use named constants with documented units.
+- No `continue` -- wrap the body in an `if` instead.


maliberty · 2026-03-06T14:19:09Z

CONTRIBUTING.md

+- No global variables. All state in classes.
+- No magic numbers. Use named constants with documented units.
+- No `continue` -- wrap the body in an `if` instead.
+- No `== true` / `== false`. Use the boolean directly.


I see varying opinions on this. Probably something to discuss further before adding (and possibly automate with clang-tidy).

maliberty · 2026-03-06T14:19:45Z

CONTRIBUTING.md

+- Use `#pragma once` not `#ifndef` guards.
+- Use `""` for project headers, `<>` only for system headers.
+- Use range-based for loops, not iterator loops.
+- Use forward declarations instead of includes where possible.


Henner has been pushing in the opposite direction.

maliberty · 2026-03-06T14:21:14Z

CONTRIBUTING.md

+- Log messages must use the OpenROAD logger (`utl::info`, `utl::warn`,
+  `utl::error`), not `printf` or `std::cout`. Use `utl::report`


utl::info (etc) are for TCL and Logger::info (etc) for C++. It would be good to clarify.

maliberty · 2026-03-06T14:22:59Z

This somewhat duplicates the other files. We should remove anything duplicative. This should appear in the read the docs

- Replace "secure-CI" with user-friendly language about maintainer-run validation on real designs - Remove `-no_init` note (handled by test runner) - Remove contested style rules: `continue`, `== true`/`== false`, forward declarations - Fix invalid `using namespace::symbol` syntax - Clarify logger API: C++ (Logger::info) vs Tcl (utl::info) - Reframe intro as concise summary pointing to detailed docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>

Replace duplicated C++ style, testing, architecture, and logging sections with references to the existing docs where this content already lives (CodingPractices.md, DeveloperGuide.md, Logger.md). Keep only the review priority ordering and AI reviewer guidance which are unique to this file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>

oharboe · 2026-03-06T14:28:45Z

Had Claude update and DRY

github-actions · 2026-03-06T14:31:26Z

clang-tidy review says "All clean, LGTM! 👍"

oharboe requested a review from maliberty March 6, 2026 06:22

gemini-code-assist bot reviewed Mar 6, 2026

View reviewed changes

maliberty requested changes Mar 6, 2026

View reviewed changes

oharboe and others added 2 commits March 6, 2026 15:26

maliberty approved these changes Mar 6, 2026

View reviewed changes

maliberty enabled auto-merge March 6, 2026 15:52

maliberty merged commit 4b3404b into The-OpenROAD-Project:master Mar 6, 2026
13 of 14 checks passed

	- Don't use `using namespace std`. Prefer `using namespace::symbol`.
	- Don't use `using namespace std;`. Prefer `using` declarations for specific symbols (e.g., `using std::vector;`).

		- Log messages must use the OpenROAD logger (`utl::info`, `utl::warn`,
		`utl::error`), not `printf` or `std::cout`. Use `utl::report`

Conversation

oharboe commented Mar 6, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 6, 2026

Uh oh!

maliberty commented Mar 6, 2026

Uh oh!

oharboe commented Mar 6, 2026

Uh oh!

maliberty commented Mar 6, 2026

Uh oh!

oharboe commented Mar 6, 2026

Uh oh!

oharboe commented Mar 6, 2026

Uh oh!

oharboe commented Mar 6, 2026

Uh oh!

maliberty commented Mar 6, 2026

Uh oh!

oharboe commented Mar 6, 2026

Uh oh!

maliberty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty commented Mar 6, 2026

Uh oh!

oharboe commented Mar 6, 2026

Uh oh!

github-actions bot commented Mar 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants